Merge pull request #1 from BruceForstall/LdpStp_Modifications_Fixes

Various fixes to ldp/stp optimization

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);
 

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)

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,25 +9127,28 @@ 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.
 
     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,23 +9158,23 @@ 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;
 
         // We don't overwrite this memory; it's simply ignored. We could zero it to be sure nobody uses it,
         // 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;
 }
 
 /*****************************************************************************

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;
 }