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.

Sign up for GitHub

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] Fixed improper peephole zero-extension removal when cdq/cwde instructions are involved #82733

Merged
merged 16 commits into from
Mar 5, 2023
Merged

[JIT] Fixed improper peephole zero-extension removal when cdq/cwde instructions are involved #82733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 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
ef8688b
Take into account cmpxchg
TIHan Feb 28, 2023
2184607
Take into account cmpxchg
TIHan Feb 28, 2023
25ac969
Feedback
TIHan Feb 28, 2023
cef781f
Added emitIsInstrWritingToReg. Removed the previously added function.
TIHan Mar 3, 2023
fa40979
Remove function from header
TIHan Mar 3, 2023
f868fa0
Quick fix
TIHan Mar 3, 2023
13c9981
Added comment
TIHan Mar 3, 2023
e6305be
Formatting
TIHan Mar 3, 2023
1248810
Feedback
TIHan Mar 3, 2023
aab0faf
Fix build
TIHan Mar 3, 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
4 changes: 4 additions & 0 deletions src/coreclr/jit/emit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2079,6 +2079,10 @@ class emitter
void emitHandleMemOp(GenTreeIndir* indir, instrDesc* id, insFormat fmt, instruction ins);
void spillIntArgRegsToShadowSlots();

#ifdef TARGET_XARCH
bool emitIsInstrWritingToReg(instrDesc* id, regNumber reg);
#endif // TARGET_XARCH

/************************************************************************/
/* The logic that creates and keeps track of instruction groups */
/************************************************************************/
Expand Down
325 changes: 218 additions & 107 deletions src/coreclr/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,125 +505,37 @@ bool emitter::AreUpper32BitsZero(regNumber reg)
bool result = false;

emitPeepholeIterateLastInstrs([&](instrDesc* id) {
switch ((ID_OPS)emitFmtToOps[id->idInsFmt()])
if (emitIsInstrWritingToReg(id, reg))
{
// This is conservative.
case ID_OP_CALL:
return PEEPHOLE_ABORT;

default:
break;
}

// This is a special case for idiv, div, imul, and mul.
// They always write to RAX and RDX.
if (instrHasImplicitRegPairDest(id->idIns()))
{
if (reg == REG_RAX || reg == REG_RDX)
{
result = (id->idOpSize() == EA_4BYTE);
return PEEPHOLE_ABORT;
}
}

switch (id->idInsFmt())
{
case IF_RWR:
case IF_RRW:

case IF_RWR_CNS:
case IF_RRW_CNS:
case IF_RRW_SHF:

case IF_RWR_RRD:
case IF_RRW_RRD:
case IF_RRW_RRW:
case IF_RRW_RRW_CNS:

case IF_RWR_RRD_RRD:
case IF_RWR_RRD_RRD_CNS:

case IF_RWR_RRD_RRD_RRD:

case IF_RWR_MRD:
case IF_RRW_MRD:
case IF_RRW_MRD_CNS:

case IF_RWR_RRD_MRD:
case IF_RWR_MRD_CNS:
case IF_RWR_RRD_MRD_CNS:
case IF_RWR_RRD_MRD_RRD:
case IF_RWR_MRD_OFF:

case IF_RWR_SRD:
case IF_RRW_SRD:
case IF_RRW_SRD_CNS:

case IF_RWR_RRD_SRD:
case IF_RWR_SRD_CNS:
case IF_RWR_RRD_SRD_CNS:
case IF_RWR_RRD_SRD_RRD:

case IF_RWR_ARD:
case IF_RRW_ARD:
case IF_RRW_ARD_CNS:

case IF_RWR_RRD_ARD:
case IF_RWR_ARD_CNS:
case IF_RWR_ARD_RRD:
case IF_RWR_RRD_ARD_CNS:
case IF_RWR_RRD_ARD_RRD:
switch (id->idIns())
{
if (id->idReg1() != reg)
{
switch (id->idInsFmt())
{
// Handles instructions who write to two registers.
case IF_RRW_RRW:
case IF_RRW_RRW_CNS:
{
if (id->idReg2() == reg)
{
result = (id->idOpSize() == EA_4BYTE);
return PEEPHOLE_ABORT;
}
break;
}

default:
break;
}

return PEEPHOLE_CONTINUE;
}

// movsx always sign extends to 8 bytes.
if (id->idIns() == INS_movsx)
{
// Conservative.
case INS_call:
return PEEPHOLE_ABORT;
}

if (id->idIns() == INS_movsxd)
{
// These instructions sign-extend.
case INS_cwde:
case INS_cdq:
case INS_movsx:
case INS_movsxd:
return PEEPHOLE_ABORT;
}

// movzx always zeroes the upper 32 bits.
if (id->idIns() == INS_movzx)
{
case INS_movzx:
result = true;
return PEEPHOLE_ABORT;
}

// otherwise rely on operation size.
result = (id->idOpSize() == EA_4BYTE);
return PEEPHOLE_ABORT;
default:
break;
}

default:
{
return PEEPHOLE_CONTINUE;
}
// otherwise rely on operation size.
result = (id->idOpSize() == EA_4BYTE);
return PEEPHOLE_ABORT;
}
else
{
return PEEPHOLE_CONTINUE;
}
});

Expand Down Expand Up @@ -677,6 +589,205 @@ bool emitter::AreUpper32BitsSignExtended(regNumber reg)
}
#endif // TARGET_64BIT

//------------------------------------------------------------------------
// emitIsInstrWritingToReg: checks if the given register is being written to
//
// Arguments:
// id - instruction of interest
// reg - register of interest
//
// Return Value:
// true if the instruction writes to the given register.
// false if it did not.
//
// Note: This only handles integer registers. Also, an INS_call will always return true.
//
bool emitter::emitIsInstrWritingToReg(instrDesc* id, regNumber reg)
{
// This only handles integer registers for now.
assert(genIsValidIntReg(reg));

instruction ins = id->idIns();

// These are special cases since they modify one or more register(s) implicitly.
switch (ins)
TIHan marked this conversation as resolved.
Show resolved Hide resolved
{
// This is conservative. We assume a call will write to all registers even if it does not.
case INS_call:
return true;

// These always write to RAX and RDX.
case INS_idiv:
case INS_div:
case INS_imulEAX:
case INS_mulEAX:
if ((reg == REG_RAX) || (reg == REG_RDX))
{
return true;
}
break;

// Always writes to RAX.
case INS_cmpxchg:
if (reg == REG_RAX)
{
return true;
}
break;

case INS_movsb:
case INS_movsd:
#ifdef TARGET_AMD64
case INS_movsq:
#endif // TARGET_AMD64
if ((reg == REG_RDI) || (reg == REG_RSI))
{
return true;
}
break;

case INS_stosb:
case INS_stosd:
#ifdef TARGET_AMD64
case INS_stosq:
#endif // TARGET_AMD64
if (reg == REG_RDI)
{
return true;
}
break;

case INS_r_movsb:
case INS_r_movsd:
#ifdef TARGET_AMD64
case INS_r_movsq:
#endif // TARGET_AMD64
if ((reg == REG_RDI) || (reg == REG_RSI) || (reg == REG_RCX))
{
return true;
}
break;

case INS_r_stosb:
case INS_r_stosd:
#ifdef TARGET_AMD64
case INS_r_stosq:
#endif // TARGET_AMD64
if ((reg == REG_RDI) || (reg == REG_RCX))
{
return true;
}
break;

default:
break;
}

#ifdef TARGET_64BIT
// This is a special case for cdq/cwde.
switch (ins)
{
case INS_cwde:
if (reg == REG_RAX)
{
return true;
}
break;

case INS_cdq:
if (reg == REG_RDX)
{
return true;
}
break;

default:
break;
}
#endif // TARGET_64BIT

switch (id->idInsFmt())
{
case IF_RWR:
case IF_RRW:

case IF_RWR_CNS:
case IF_RRW_CNS:
case IF_RRW_SHF:

case IF_RWR_RRD:
case IF_RRW_RRD:
case IF_RRW_RRW:
case IF_RRW_RRW_CNS:

case IF_RWR_RRD_RRD:
case IF_RWR_RRD_RRD_CNS:

case IF_RWR_RRD_RRD_RRD:

case IF_RWR_MRD:
case IF_RRW_MRD:
case IF_RRW_MRD_CNS:

case IF_RWR_RRD_MRD:
case IF_RWR_MRD_CNS:
case IF_RWR_RRD_MRD_CNS:
case IF_RWR_RRD_MRD_RRD:
case IF_RWR_MRD_OFF:

case IF_RWR_SRD:
case IF_RRW_SRD:
case IF_RRW_SRD_CNS:

case IF_RWR_RRD_SRD:
case IF_RWR_SRD_CNS:
case IF_RWR_RRD_SRD_CNS:
case IF_RWR_RRD_SRD_RRD:

case IF_RWR_ARD:
case IF_RRW_ARD:
case IF_RRW_ARD_CNS:

case IF_RWR_RRD_ARD:
case IF_RWR_ARD_CNS:
case IF_RWR_ARD_RRD:
case IF_RWR_RRD_ARD_CNS:
case IF_RWR_RRD_ARD_RRD:
{
if (id->idReg1() != reg)
{
switch (id->idInsFmt())
{
// Handles instructions who write to two registers.
case IF_RRW_RRW:
case IF_RRW_RRW_CNS:
{
if (id->idReg2() == reg)
{
return true;
}
break;
}

default:
break;
}

return false;
}

return true;
}

default:
{
return false;
}
}

return false;
}

//------------------------------------------------------------------------
// 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
Expand Down
Loading