Skip to content

Commit

Permalink
JIT: fix gc hole in peephole optimizations (#78074)
Browse files Browse the repository at this point in the history 
We cannot safely peephole instructions that straddle a gc enable boundary.
Detecting when this might happen is a bit subtle; currently we rely on
`emitForceNewIG` to be set.

Add a new utility 'emitCanPeepholeLastIns` to centralize the logic that
decides whether basing current emission on `emitLastIns` is safe.

Closes #77661.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Nov 9, 2022
1 parent 3bcdb34 commit af494f4
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 24 deletions.
14 changes: 14 additions & 0 deletions src/coreclr/jit/emit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2180,6 +2180,20 @@ class emitter

instrDesc* emitLastIns;

// Check if a peephole optimization involving emitLastIns is safe.
//
// We must have a lastInstr to consult.
// The emitForceNewIG check here prevents peepholes from crossing nogc boundaries.
// The final check prevents looking across an IG boundary unless we're in an extension IG.
bool emitCanPeepholeLastIns()
{
return (emitLastIns != nullptr) && // there is an emitLastInstr
!emitForceNewIG && // and we're not about to start a new IG
((emitCurIGinsCnt > 0) || // and we're not at the start of a new IG
((emitCurIG->igFlags & IGF_EXTEND) != 0)); // or we are at the start of a new IG,
// and it's an extension IG
}

#ifdef TARGET_ARMARCH
instrDesc* emitLastMemBarrier;
#endif
Expand Down
13 changes: 5 additions & 8 deletions src/coreclr/jit/emitarm64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15957,7 +15957,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN
return false;
}

const bool isFirstInstrInBlock = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0);
const bool canOptimize = emitCanPeepholeLastIns();

if (dst == src)
{
Expand All @@ -15977,17 +15977,16 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN
else if (isGeneralRegisterOrSP(dst) && (size == EA_4BYTE))
{
// See if the previous instruction already cleared upper 4 bytes for us unintentionally
if (!isFirstInstrInBlock && (emitLastIns != nullptr) && (emitLastIns->idReg1() == dst) &&
(emitLastIns->idOpSize() == size) && emitLastIns->idInsIs(INS_ldr, INS_ldrh, INS_ldrb))
if (canOptimize && (emitLastIns->idReg1() == dst) && (emitLastIns->idOpSize() == size) &&
emitLastIns->idInsIs(INS_ldr, INS_ldrh, INS_ldrb))
{
JITDUMP("\n -- suppressing mov because ldr already cleared upper 4 bytes\n");
return true;
}
}
}

if (!isFirstInstrInBlock && // Don't optimize if instruction is not the first instruction in IG.
(emitLastIns != nullptr) &&
if (canOptimize && // Don't optimize if unsafe.
(emitLastIns->idIns() == INS_mov) && // Don't optimize if last instruction was not 'mov'.
(emitLastIns->idOpSize() == size)) // Don't optimize if operand size is different than previous instruction.
{
Expand Down Expand Up @@ -16065,9 +16064,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN
bool emitter::IsRedundantLdStr(
instruction ins, regNumber reg1, regNumber reg2, ssize_t imm, emitAttr size, insFormat fmt)
{
bool isFirstInstrInBlock = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0);

if (((ins != INS_ldr) && (ins != INS_str)) || (isFirstInstrInBlock) || (emitLastIns == nullptr))
if (((ins != INS_ldr) && (ins != INS_str)) || !emitCanPeepholeLastIns())
{
return false;
}
Expand Down
28 changes: 12 additions & 16 deletions src/coreclr/jit/emitxarch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,10 +486,9 @@ bool emitter::IsFlagsAlwaysModified(instrDesc* id)

bool emitter::AreUpper32BitsZero(regNumber reg)
{
// If there are no instructions in this IG, we can look back at
// the previous IG's instructions if this IG is an extension.
// Only consider if safe
//
if ((emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0))
if (!emitCanPeepholeLastIns())
{
return false;
}
Expand Down Expand Up @@ -569,8 +568,9 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr
return false;
}

// Don't look back across IG boundaries (possible control flow)
if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0))
// Only consider if safe
//
if (!emitCanPeepholeLastIns())
{
return false;
}
Expand Down Expand Up @@ -652,8 +652,9 @@ bool emitter::AreFlagsSetForSignJumpOpt(regNumber reg, emitAttr opSize, GenTree*
return false;
}

// Don't look back across IG boundaries (possible control flow)
if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0))
// Only consider if safe
//
if (!emitCanPeepholeLastIns())
{
return false;
}
Expand Down Expand Up @@ -5717,13 +5718,10 @@ bool emitter::IsRedundantMov(
return true;
}

bool isFirstInstrInBlock = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0);

// TODO-XArch-CQ: Certain instructions, such as movaps vs movups, are equivalent in
// functionality even if their actual identifier differs and we should optimize these

if (isFirstInstrInBlock || // Don't optimize if instruction is the first instruction in IG.
(emitLastIns == nullptr) || // or if a last instruction doesn't exist
if (!emitCanPeepholeLastIns() || // Don't optimize if unsafe
(emitLastIns->idIns() != ins) || // or if the instruction is different from the last instruction
(emitLastIns->idOpSize() != size) || // or if the operand size is different from the last instruction
(emitLastIns->idInsFmt() != fmt)) // or if the format is different from the last instruction
Expand Down Expand Up @@ -8410,14 +8408,10 @@ bool emitter::IsRedundantStackMov(instruction ins, insFormat fmt, emitAttr size,
return false;
}

bool hasSideEffect = HasSideEffect(ins, size);

bool isFirstInstrInBlock = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0);
// TODO-XArch-CQ: Certain instructions, such as movaps vs movups, are equivalent in
// functionality even if their actual identifier differs and we should optimize these

if (isFirstInstrInBlock || // Don't optimize if instruction is the first instruction in IG.
(emitLastIns == nullptr) || // or if a last instruction doesn't exist
if (!emitCanPeepholeLastIns() || // Don't optimize if unsafe
(emitLastIns->idIns() != ins) || // or if the instruction is different from the last instruction
(emitLastIns->idOpSize() != size)) // or if the operand size is different from the last instruction
{
Expand All @@ -8434,6 +8428,8 @@ bool emitter::IsRedundantStackMov(instruction ins, insFormat fmt, emitAttr size,
int varNum = emitLastIns->idAddr()->iiaLclVar.lvaVarNum();
int lastOffs = emitLastIns->idAddr()->iiaLclVar.lvaOffset();

const bool hasSideEffect = HasSideEffect(ins, size);

// Check if the last instruction and current instructions use the same register and local memory.
if (varNum == varx && lastReg1 == ireg && lastOffs == offs)
{
Expand Down

0 comments on commit af494f4

Please sign in to comment.