Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JIT] X86/X64 - Eliminate redundant 'cmp' instructions #82750

Merged
merged 33 commits into from
Apr 5, 2023
Merged
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
5f12506
Fixed improper peephole zero-extension removal when cdq/cdqe/cwde ins…
TIHan Feb 27, 2023
cf0bdf2
Update regression test
TIHan Feb 27, 2023
697dfdc
Formatting
TIHan Feb 27, 2023
f78c28d
Handle cdq differently
TIHan Feb 27, 2023
b6467cb
Handle cdq differently
TIHan Feb 27, 2023
334fc46
Handle cdq differently
TIHan Feb 27, 2023
47eb762
Initial commit to eliminate redundant 'cmp' instructions
TIHan Feb 28, 2023
ef8688b
Take into account cmpxchg
TIHan Feb 28, 2023
2184607
Take into account cmpxchg
TIHan Feb 28, 2023
25ac969
Feedback
TIHan Feb 28, 2023
2cc3541
Temporarily disable cmp opt if we encounter a mov
TIHan Mar 1, 2023
1013666
Merge remote-tracking branch 'upstream/main' into redundant-cmp-opt
TIHan Mar 1, 2023
15e6462
Merge branch 'redundant-cmp-opt' of https://github.com/TIHan/runtime …
TIHan Mar 1, 2023
3218581
Allow checking for mov
TIHan Mar 1, 2023
e5e2599
Allow regardless of targetReg
TIHan Mar 1, 2023
baee67f
Allow regardless of targetReg
TIHan Mar 1, 2023
0827dc3
Merge branch 'zero-extend-fix' into redundant-cmp-opt
TIHan Mar 1, 2023
03e4f8b
Checking if an instruction resets a flag.
TIHan Mar 1, 2023
5058e59
Remove useless comment
TIHan Mar 1, 2023
0ba01ef
Minor fix
TIHan Mar 1, 2023
8d8485e
Abort are checking cmp
TIHan Mar 2, 2023
49b6f27
Some refactoring. Taking into account any instruction that modifies f…
TIHan Mar 3, 2023
49a8043
Minor cleanup
TIHan Mar 3, 2023
ee0cd47
Remove function from header
TIHan Mar 3, 2023
91c1054
Quick fix
TIHan Mar 3, 2023
1c5f518
Sync
TIHan Mar 4, 2023
f74a895
Merge
TIHan Mar 6, 2023
61ee9da
Merge remote-tracking branch 'upstream/main' into redundant-cmp-opt
TIHan Apr 3, 2023
4d4e059
Formatting
TIHan Apr 3, 2023
03d9ab3
Only look for 'cmp reg, reg'
TIHan Apr 5, 2023
51c70a7
Added comment
TIHan Apr 5, 2023
3dd723f
Update src/coreclr/jit/emitxarch.cpp
TIHan Apr 5, 2023
0cb00b4
Update src/coreclr/jit/emitxarch.cpp
TIHan Apr 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Initial commit to eliminate redundant 'cmp' instructions
TIHan committed Feb 28, 2023
commit 47eb762a4acd4c9b132a6e0de8616f9d0716f153
10 changes: 9 additions & 1 deletion src/coreclr/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
@@ -6747,7 +6747,15 @@ void CodeGen::genCompareInt(GenTree* treeNode)
targetType = TYP_BOOL; // just a tip for inst_SETCC that movzx is not needed
}
}
emit->emitInsBinary(ins, emitTypeSize(type), op1, op2);

emitAttr size = emitTypeSize(type);
bool canSkip = compiler->opts.OptimizationEnabled() && (ins == INS_cmp) && !op1->isUsedFromMemory() &&
!op2->isUsedFromMemory() && emit->IsRedundantCmp(size, op1->GetRegNum(), op2->GetRegNum());

if (!canSkip)
{
emit->emitInsBinary(ins, size, op1, op2);
}
}

// Are we evaluating this into a register?
110 changes: 110 additions & 0 deletions src/coreclr/jit/emitxarch.cpp
Original file line number Diff line number Diff line change
@@ -677,6 +677,116 @@ bool emitter::AreUpper32BitsSignExtended(regNumber reg)
}
#endif // TARGET_64BIT

bool emitter::IsRedundantCmp(emitAttr size, regNumber reg1, regNumber reg2)
{
// Only allow GPRs.
// If not a valid register, then return false.
if (!genIsValidIntReg(reg1))
return false;

if (!genIsValidIntReg(reg2))
return false;

// Only consider if safe
//
if (!emitCanPeepholeLastIns())
{
return false;
}

bool result = false;

//instInfo
emitPeepholeIterateLastInstrs([&](instrDesc* id) {
switch (id->idIns())
{
case INS_seta:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to specify specific instructions - I'm just doing this for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you get this list from?

Copy link
Contributor Author

@TIHan TIHan Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just aware of the set and jmp instructions and we have the list defined in our instruction header file, so I just copied it from there.

case INS_setae:
case INS_setb:
case INS_setbe:
case INS_sete:
case INS_setg:
case INS_setge:
case INS_setl:
case INS_setle:
case INS_setne:
case INS_setno:
case INS_setnp:
case INS_setns:
case INS_seto:
case INS_setp:
case INS_sets:

case INS_tail_i_jmp:
case INS_i_jmp:
case INS_jmp:
case INS_jo:
case INS_jno:
case INS_jb:
case INS_jae:
case INS_je:
case INS_jne:
case INS_jbe:
case INS_ja:
case INS_js:
case INS_jns:
case INS_jp:
case INS_jnp:
case INS_jl:
case INS_jge:
case INS_jle:
case INS_jg:

case INS_l_jmp:
case INS_l_jo:
case INS_l_jno:
case INS_l_jb:
case INS_l_jae:
case INS_l_je:
case INS_l_jne:
case INS_l_jbe:
case INS_l_ja:
case INS_l_js:
case INS_l_jns:
case INS_l_jp:
case INS_l_jnp:
case INS_l_jl:
case INS_l_jge:
case INS_l_jle:
case INS_l_jg:
TIHan marked this conversation as resolved.
Show resolved Hide resolved
return PEEPHOLE_CONTINUE;

case INS_mov:
case INS_movsx:
case INS_movzx:
{
if ((id->idReg1() == reg1) || (id->idReg1() == reg2))
TIHan marked this conversation as resolved.
Show resolved Hide resolved
{
return PEEPHOLE_ABORT;
}

return PEEPHOLE_CONTINUE;
}

case INS_cmp:
{
if ((id->idReg1() == reg1) && (id->idReg2() == reg2))
{
result = (size == id->idOpSize());
return PEEPHOLE_ABORT;
}

return PEEPHOLE_ABORT;
}

default:
return PEEPHOLE_ABORT;
}
});

return result;
}

//------------------------------------------------------------------------
// AreFlagsSetToZeroCmp: Checks if the previous instruction set the SZ, and optionally OC, flags to
// the same values as if there were a compare to 0
2 changes: 2 additions & 0 deletions src/coreclr/jit/emitxarch.h
Original file line number Diff line number Diff line change
@@ -132,6 +132,8 @@ bool AreUpper32BitsZero(regNumber reg);
bool AreUpper32BitsSignExtended(regNumber reg);
#endif // TARGET_64BIT

bool IsRedundantCmp(emitAttr size, regNumber reg1, regNumber reg2);

bool AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, GenCondition cond);
bool AreFlagsSetForSignJumpOpt(regNumber reg, emitAttr opSize, GenCondition cond);