From 50f8b6005fd51dd004e04560fe25ab3904cd13b5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 8 Nov 2022 13:27:50 -0800 Subject: [PATCH 1/2] JIT: fix gc hole in peephole optimizations 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. Closed #77661. --- src/coreclr/jit/emit.h | 7 +++++++ src/coreclr/jit/emitarm64.cpp | 10 ++++------ src/coreclr/jit/emitxarch.cpp | 18 +++++++----------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index c5d245ba83c66d..e3478f4f495c13 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2179,6 +2179,13 @@ class emitter instrDesc* emitLastIns; + // Check if a peephole opt involving emitLastIns is unsafe + // emitForceNewIG here prevents peepholes from crossing nogc boundaries. + bool emitCannotPeepholeLastIns() + { + return emitForceNewIG || ((emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0)); + } + #ifdef TARGET_ARMARCH instrDesc* emitLastMemBarrier; #endif diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 499b0470ff298e..6fb02fd1a1142f 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -15940,7 +15940,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 = !emitCannotPeepholeLastIns(); if (dst == src) { @@ -15960,7 +15960,7 @@ 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) && + if (canOptimize && (emitLastIns != nullptr) && (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"); @@ -15969,7 +15969,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN } } - if (!isFirstInstrInBlock && // Don't optimize if instruction is not the first instruction in IG. + if (canOptimize && // Don't optimize if instruction is not the first instruction in IG. (emitLastIns != nullptr) && (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. @@ -16048,9 +16048,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)) || emitCannotPeepholeLastIns()) { return false; } diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index cbba666f80eb7b..6a8bc4f055b3ad 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -317,7 +317,7 @@ 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. // - if ((emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0)) + if (emitCannotPeepholeLastIns()) { return false; } @@ -398,7 +398,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr } // Don't look back across IG boundaries (possible control flow) - if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0)) + if (emitCannotPeepholeLastIns()) { return false; } @@ -480,8 +480,7 @@ 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)) + if (emitCannotPeepholeLastIns()) { return false; } @@ -4698,12 +4697,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. + if (emitCannotPeepholeLastIns() || // Don't optimize if instruction is the first instruction in IG. (emitLastIns == nullptr) || // or if a last instruction doesn't exist (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 @@ -7343,13 +7340,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. + if (emitCannotPeepholeLastIns() || // Don't optimize if instruction is the first instruction in IG. (emitLastIns == nullptr) || // or if a last instruction doesn't exist (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 @@ -7367,6 +7361,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) { From 1c4bca1fad48be273c2c737b3e80e3b9dbb0abdd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 8 Nov 2022 18:37:32 -0800 Subject: [PATCH 2/2] revise per feedback --- src/coreclr/jit/emit.h | 17 ++++++++++++----- src/coreclr/jit/emitarm64.cpp | 11 +++++------ src/coreclr/jit/emitxarch.cpp | 20 ++++++++++---------- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index e3478f4f495c13..92fb93df931b47 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2179,11 +2179,18 @@ class emitter instrDesc* emitLastIns; - // Check if a peephole opt involving emitLastIns is unsafe - // emitForceNewIG here prevents peepholes from crossing nogc boundaries. - bool emitCannotPeepholeLastIns() - { - return emitForceNewIG || ((emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0)); + // 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 diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 6fb02fd1a1142f..f94c703ed39225 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -15940,7 +15940,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN return false; } - const bool canOptimize = !emitCannotPeepholeLastIns(); + const bool canOptimize = emitCanPeepholeLastIns(); if (dst == src) { @@ -15960,8 +15960,8 @@ 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 (canOptimize && (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; @@ -15969,8 +15969,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN } } - if (canOptimize && // 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. { @@ -16048,7 +16047,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) { - if (((ins != INS_ldr) && (ins != INS_str)) || emitCannotPeepholeLastIns()) + if (((ins != INS_ldr) && (ins != INS_str)) || !emitCanPeepholeLastIns()) { return false; } diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 6a8bc4f055b3ad..86cf648e7302b4 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -314,10 +314,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 (emitCannotPeepholeLastIns()) + if (!emitCanPeepholeLastIns()) { return false; } @@ -397,8 +396,9 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr return false; } - // Don't look back across IG boundaries (possible control flow) - if (emitCannotPeepholeLastIns()) + // Only consider if safe + // + if (!emitCanPeepholeLastIns()) { return false; } @@ -480,7 +480,9 @@ bool emitter::AreFlagsSetForSignJumpOpt(regNumber reg, emitAttr opSize, GenTree* return false; } - if (emitCannotPeepholeLastIns()) + // Only consider if safe + // + if (!emitCanPeepholeLastIns()) { return false; } @@ -4700,8 +4702,7 @@ bool emitter::IsRedundantMov( // 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 (emitCannotPeepholeLastIns() || // 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 @@ -7343,8 +7344,7 @@ bool emitter::IsRedundantStackMov(instruction ins, insFormat fmt, emitAttr size, // 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 (emitCannotPeepholeLastIns() || // 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 {