diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index c860fa4d79cda..cc086147e0ead 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -664,12 +664,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX */ #ifdef DEBUG - void genIPmappingDisp(unsigned mappingNum, IPmappingDsc* ipMapping); + void genIPmappingDisp(unsigned mappingNum, const IPmappingDsc* ipMapping); void genIPmappingListDisp(); #endif // DEBUG void genIPmappingAdd(IPmappingDscKind kind, const DebugInfo& di, bool isLabel); void genIPmappingAddToFront(IPmappingDscKind kind, const DebugInfo& di, bool isLabel); + void genIPmappingUpdateForRemovedInstruction(emitLocation loc, unsigned removedCodeSize); void genIPmappingGen(); void genAddRichIPMappingHere(const DebugInfo& di); diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index dcd69bd5a974f..72f3f2f23022b 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -7166,7 +7166,7 @@ const char* CodeGen::siStackVarName(size_t offs, size_t size, unsigned reg, unsi * Display a IPmappingDsc. Pass -1 as mappingNum to not display a mapping number. */ -void CodeGen::genIPmappingDisp(unsigned mappingNum, IPmappingDsc* ipMapping) +void CodeGen::genIPmappingDisp(unsigned mappingNum, const IPmappingDsc* ipMapping) { if (mappingNum != unsigned(-1)) { @@ -7318,6 +7318,57 @@ void CodeGen::genIPmappingAddToFront(IPmappingDscKind kind, const DebugInfo& di, #endif // DEBUG } +//------------------------------------------------------------------------ +// genIPmappingUpdateForRemovedInstruction: Update the IP mapping table for a removed instruction. +// If the last IP mapping corresponds to the location immediately after the removed instruction, then +// update the mapping to the location before the removed instruction. +// +// Arguments: +// loc - the emitter location we compare against +// removedCodeSize - the size of the instruction being removed +// +void CodeGen::genIPmappingUpdateForRemovedInstruction(emitLocation loc, unsigned removedCodeSize) +{ + if (!compiler->opts.compDbgInfo) + { + return; + } + + if (compiler->genIPmappings.size() <= 0) + { + return; + } + + IPmappingDsc& prev = compiler->genIPmappings.back(); + if (prev.ipmdNativeLoc == loc) + { + assert(prev.ipmdKind == IPmappingDscKind::Normal); + +#ifdef DEBUG + if (verbose) + { + printf("Updating last IP mapping: "); + genIPmappingDisp(unsigned(-1), &prev); + } +#endif // DEBUG + + int newInsCount = prev.ipmdNativeLoc.GetInsNum() - 1; + int newInsOffset = prev.ipmdNativeLoc.GetInsOffset() - removedCodeSize; + assert(newInsCount >= 0); + assert(newInsOffset >= 0); + unsigned newLoc = GetEmitter()->emitSpecifiedOffset(newInsCount, newInsOffset); + prev.ipmdNativeLoc.SetLocation(prev.ipmdNativeLoc.GetIG(), newLoc); + +#ifdef DEBUG + if (verbose) + { + printf(" to: "); + genIPmappingDisp(unsigned(-1), &prev); + } +#endif // DEBUG + } +} + /*****************************************************************************/ void CodeGen::genEnsureCodeEmitted(const DebugInfo& di) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 1018a8f156d2f..01ad1546df7a4 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -33,6 +33,14 @@ void emitLocation::CaptureLocation(emitter* emit) assert(Valid()); } +void emitLocation::SetLocation(insGroup* _ig, unsigned _codePos) +{ + ig = _ig; + codePos = _codePos; + + assert(Valid()); +} + bool emitLocation::IsCurrentLocation(emitter* emit) const { assert(Valid()); @@ -50,6 +58,11 @@ int emitLocation::GetInsNum() const return emitGetInsNumFromCodePos(codePos); } +int emitLocation::GetInsOffset() const +{ + return emitGetInsOfsFromCodePos(codePos); +} + // Get the instruction offset in the current instruction group, which must be a funclet prolog group. // This is used to find an instruction offset used in unwind data. // TODO-AMD64-Bug?: We only support a single main function prolog group, but allow for multiple funclet prolog @@ -1002,28 +1015,26 @@ insGroup* emitter::emitSavIG(bool emitAdd) } } - // Fix the last instruction field + // Fix the last instruction field, if set. Note that even if there are instructions in the IG, + // emitLastIns might not be set if an optimization has just deleted it, and the new instruction + // being adding causes a new EXTEND IG to be created. Also, emitLastIns might not be in this IG + // at all if this IG is empty. - if (sz != 0) + assert((emitLastIns == nullptr) == (emitLastInsIG == nullptr)); + if ((emitLastIns != nullptr) && (sz != 0)) { - assert(emitLastIns != nullptr); + // If we get here, emitLastIns must be in the current IG we are saving. assert(emitLastInsIG == emitCurIG); assert(emitCurIGfreeBase <= (BYTE*)emitLastIns); assert((BYTE*)emitLastIns < emitCurIGfreeBase + sz); #if defined(TARGET_XARCH) - assert(emitLastIns != nullptr); if (emitLastIns->idIns() == INS_jmp) { ig->igFlags |= IGF_HAS_REMOVABLE_JMP; } #endif - // Make a copy of "emitLastIns", to be used in the event that a future - // optimisation call for the removal of a previously-emitted instruction. - - emitPrevLastIns = emitLastIns; - emitLastIns = (instrDesc*)((BYTE*)id + ((BYTE*)emitLastIns - (BYTE*)emitCurIGfreeBase)); emitLastInsIG = ig; } @@ -1174,9 +1185,8 @@ void emitter::emitBegFN(bool hasFramePtr emitPrologIG = emitIGlist = emitIGlast = emitCurIG = ig = emitAllocIG(); - emitLastIns = nullptr; - emitLastInsIG = nullptr; - emitPrevLastIns = nullptr; + emitLastIns = nullptr; + emitLastInsIG = nullptr; #ifdef TARGET_ARMARCH emitLastMemBarrier = nullptr; @@ -1497,11 +1507,6 @@ void* emitter::emitAllocAnyInstr(size_t sz, emitAttr opsz) /* Grab the space for the instruction */ - // Make a copy of "emitLastIns", to be used in the event that a future - // optimisation calls for the removal of a previously-emitted instruction. - - emitPrevLastIns = emitLastIns; - emitLastIns = id = (instrDesc*)(emitCurIGfreeNext + m_debugInfoSize); emitLastInsIG = emitCurIG; emitCurIGfreeNext += fullSize; @@ -9101,6 +9106,19 @@ void emitter::emitNxtIG(bool extend) // The `emitLastIns` is set to nullptr after this function. It is expected that a new instruction // will be immediately generated after this, which will set it again. // +// Removing an instruction can invalidate any captured emitter location (using emitLocation::CaptureLocation()) +// after the instruction was generated. This is because the emitLocation stores the current IG instruction +// number and code size. If the instruction is removed and not replaced (e.g., it is at the end of the IG, +// and any replacement creates a new EXTEND IG), then the saved instruction number is incorrect. One way to +// work around this problem might be to make emitter::emitCodeOffset() tolerant of too-large instruction numbers. +// However, this doesn't fix the real problem. If an instruction is generated, then an emitLocation is captured, +// then the instruction is removed and replaced, the captured emitLocation will be pointing *after* the new +// instruction. Semantically, it probably should be pointing *before* the new instruction. The main case +// where emitLocations are captured, and this problem occurs, is in genIPmappingAdd(). So, if the removed +// instruction is just before the last added IP mapping, then update the IP mapping. This assumes that there +// will be only one such mapping (and not a set of equivalent, same-location mappings), which would require +// looping over the mappings. +// // NOTE: It is expected that the GC effect of the removed instruction will be handled by the newly // generated replacement(s). // @@ -9109,7 +9127,7 @@ void emitter::emitRemoveLastInstruction() assert(emitLastIns != nullptr); assert(emitLastInsIG != nullptr); - JITDUMP("Removing saved instruction in %s:\n", emitLabelString(emitLastInsIG)); + JITDUMP("Removing saved instruction in %s:\n> ", emitLabelString(emitLastInsIG)); JITDUMPEXEC(dispIns(emitLastIns)) // We should assert it's not a jmp, as that would require updating the jump lists, e.g. emitCurIGjmpList. @@ -9117,17 +9135,20 @@ void emitter::emitRemoveLastInstruction() BYTE* lastInsActualStartAddr = (BYTE*)emitLastIns - m_debugInfoSize; unsigned short lastCodeSize = (unsigned short)emitLastIns->idCodeSize(); - if ((emitCurIGfreeBase <= (BYTE*)lastInsActualStartAddr) && ((BYTE*)lastInsActualStartAddr < emitCurIGfreeEndp)) + if ((emitCurIGfreeBase <= lastInsActualStartAddr) && (lastInsActualStartAddr < emitCurIGfreeEndp)) { // The last instruction is in the current buffer. That means the current IG is non-empty. assert(emitCurIGnonEmpty()); - assert((BYTE*)lastInsActualStartAddr < emitCurIGfreeNext); + assert(lastInsActualStartAddr < emitCurIGfreeNext); assert(emitCurIGinsCnt >= 1); assert(emitCurIGsize >= emitLastIns->idCodeSize()); - size_t insSize = emitCurIGfreeNext - (BYTE*)lastInsActualStartAddr; + // Use an emit location representing the current location (before the instruction is removed). + codeGen->genIPmappingUpdateForRemovedInstruction(emitLocation(this), emitLastIns->idCodeSize()); + + size_t insSize = emitCurIGfreeNext - lastInsActualStartAddr; - emitCurIGfreeNext = (BYTE*)lastInsActualStartAddr; + emitCurIGfreeNext = lastInsActualStartAddr; emitCurIGinsCnt -= 1; emitCurIGsize -= lastCodeSize; @@ -9137,11 +9158,14 @@ void emitter::emitRemoveLastInstruction() else { // The last instruction has already been saved. It must be the last instruction in the group. - assert((BYTE*)emitLastInsIG->igData + emitLastInsIG->igDataSize == - (BYTE*)emitLastIns + emitSizeOfInsDsc(emitLastIns)); + assert(emitLastInsIG->igData + emitLastInsIG->igDataSize == (BYTE*)emitLastIns + emitSizeOfInsDsc(emitLastIns)); assert(emitLastInsIG->igInsCnt >= 1); assert(emitLastInsIG->igSize >= lastCodeSize); + // Create an emit location representing the end of the saved IG. + emitLocation loc(emitLastInsIG, emitSpecifiedOffset(emitLastInsIG->igInsCnt, emitLastInsIG->igSize)); + codeGen->genIPmappingUpdateForRemovedInstruction(loc, emitLastIns->idCodeSize()); + emitLastInsIG->igInsCnt -= 1; emitLastInsIG->igSize -= lastCodeSize; @@ -9149,11 +9173,8 @@ void emitter::emitRemoveLastInstruction() // but it's not necessary, and leaving it might be useful for debugging. } - // As we are removing the instruction, so we need to wind back - // "emitLastIns" to its previous value. - - emitLastIns = emitPrevLastIns; - emitPrevLastIns = nullptr; + emitLastIns = nullptr; + emitLastInsIG = nullptr; } /***************************************************************************** diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index ab725413ab416..45dca5be43c6b 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -131,6 +131,16 @@ class emitLocation { } + emitLocation(insGroup* _ig, unsigned _codePos) + { + SetLocation(_ig, _codePos); + } + + emitLocation(emitter* emit) + { + CaptureLocation(emit); + } + emitLocation(void* emitCookie) : ig((insGroup*)emitCookie), codePos(0) { } @@ -142,6 +152,7 @@ class emitLocation } void CaptureLocation(emitter* emit); + void SetLocation(insGroup* _ig, unsigned _codePos); bool IsCurrentLocation(emitter* emit) const; @@ -160,6 +171,7 @@ class emitLocation } int GetInsNum() const; + int GetInsOffset() const; bool operator!=(const emitLocation& other) const { @@ -2178,20 +2190,23 @@ class emitter instrDesc* emitLastIns; insGroup* emitLastInsIG; - instrDesc* emitPrevLastIns; // Check if a peephole optimization involving emitLastIns is safe. // - // We must have a lastInstr to consult. + // We must have a non-null emitLastIns 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 + assert((emitLastIns == nullptr) == (emitLastInsIG == nullptr)); + + 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 + ((emitLastInsIG->igFlags & IGF_NOGCINTERRUPT) == (emitCurIG->igFlags & IGF_NOGCINTERRUPT)); + // and the last instr IG has the same GC interrupt status as the current IG } #ifdef TARGET_ARMARCH @@ -2821,12 +2836,15 @@ inline unsigned emitGetInsOfsFromCodePos(unsigned codePos) inline unsigned emitter::emitCurOffset() { - unsigned codePos = emitCurIGinsCnt + (emitCurIGsize << 16); + return emitSpecifiedOffset(emitCurIGinsCnt, emitCurIGsize); +} - assert(emitGetInsOfsFromCodePos(codePos) == emitCurIGsize); - assert(emitGetInsNumFromCodePos(codePos) == emitCurIGinsCnt); +inline unsigned emitter::emitSpecifiedOffset(unsigned insCount, unsigned igSize) +{ + unsigned codePos = insCount + (igSize << 16); - // printf("[IG=%02u;ID=%03u;OF=%04X] => %08X\n", emitCurIG->igNum, emitCurIGinsCnt, emitCurIGsize, codePos); + assert(emitGetInsOfsFromCodePos(codePos) == igSize); + assert(emitGetInsNumFromCodePos(codePos) == insCount); return codePos; } diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 860e7b7ddd6fc..41b2a73ba5958 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -5672,7 +5672,7 @@ void emitter::emitIns_R_R_I( // For volatile load/store, there will be memory barrier instruction before/after the load/store // and in such case, IsRedundantLdStr() returns false, because the method just checks for load/store // pair next to each other. - if (emitComp->opts.OptimizationEnabled() && IsRedundantLdStr(ins, reg1, reg2, imm, size, fmt)) + if (IsRedundantLdStr(ins, reg1, reg2, imm, size, fmt)) { return; } @@ -7671,7 +7671,7 @@ void emitter::emitIns_R_S(instruction ins, emitAttr attr, regNumber reg1, int va } // Is the ldr/str even necessary? - if (emitComp->opts.OptimizationEnabled() && IsRedundantLdStr(ins, reg1, reg2, imm, size, fmt)) + if (IsRedundantLdStr(ins, reg1, reg2, imm, size, fmt)) { return; } @@ -7911,7 +7911,7 @@ void emitter::emitIns_S_R(instruction ins, emitAttr attr, regNumber reg1, int va } // Is the ldr/str even necessary? - if (emitComp->opts.OptimizationEnabled() && IsRedundantLdStr(ins, reg1, reg2, imm, size, fmt)) + if (IsRedundantLdStr(ins, reg1, reg2, imm, size, fmt)) { return; } @@ -16090,7 +16090,17 @@ 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)) || !emitCanPeepholeLastIns()) + if (!emitComp->opts.OptimizationEnabled()) + { + return false; + } + + if (!emitCanPeepholeLastIns()) + { + return false; + } + + if ((ins != INS_ldr) && (ins != INS_str)) { return false; } @@ -16168,6 +16178,7 @@ bool emitter::IsRedundantLdStr( // // Return Value: // "true" if the previous instruction HAS been overwritten. +// bool emitter::TryReplaceLdrStrWithPairInstr( instruction ins, emitAttr reg1Attr, regNumber reg1, regNumber reg2, ssize_t imm, emitAttr size, insFormat fmt) { @@ -16176,6 +16187,11 @@ bool emitter::TryReplaceLdrStrWithPairInstr( return false; } + if (!emitCanPeepholeLastIns()) + { + return false; + } + // Register 2 needs conversion to unencoded value. reg2 = encodingZRtoSP(reg2); @@ -16248,14 +16264,13 @@ bool emitter::TryReplaceLdrStrWithPairInstr( // eRO_none - No optimization of consecutive instructions is possible // eRO_ascending - Registers can be loaded/ stored into ascending store locations // eRO_descending - Registers can be loaded/ stored into decending store locations. - +// emitter::RegisterOrder emitter::IsOptimizableLdrStr( instruction ins, regNumber reg1, regNumber reg2, ssize_t imm, emitAttr size, insFormat fmt) { - bool isFirstInstrInBlock = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0); - RegisterOrder optimisationOrder = eRO_none; + RegisterOrder optimisationOrder = eRO_none; - if (((ins != INS_ldr) && (ins != INS_str)) || !emitCanPeepholeLastIns()) + if ((ins != INS_ldr) && (ins != INS_str)) { return eRO_none; } diff --git a/src/coreclr/jit/emitloongarch64.h b/src/coreclr/jit/emitloongarch64.h index 2dd9d63750289..fcbb32fa7d17f 100644 --- a/src/coreclr/jit/emitloongarch64.h +++ b/src/coreclr/jit/emitloongarch64.h @@ -73,8 +73,6 @@ unsigned emitOutput_Instr(BYTE* dst, code_t code); // If yes, the caller of this method can choose to omit current mov instruction. static bool IsMovInstruction(instruction ins); bool IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regNumber src, bool canSkip); -bool IsRedundantLdStr( - instruction ins, regNumber reg1, regNumber reg2, ssize_t imm, emitAttr size, insFormat fmt); // New functions end. /************************************************************************/ /* Public inline informational methods */ diff --git a/src/coreclr/jit/emitpub.h b/src/coreclr/jit/emitpub.h index 723d9ca0563c9..0133fb19f0212 100644 --- a/src/coreclr/jit/emitpub.h +++ b/src/coreclr/jit/emitpub.h @@ -64,6 +64,7 @@ void emitFinishPrologEpilogGeneration(); void* emitCurBlock(); unsigned emitCurOffset(); +unsigned emitSpecifiedOffset(unsigned insCount, unsigned igSize); UNATIVE_OFFSET emitCodeOffset(void* blockPtr, unsigned codeOffs);