[DebugInfo][RemoveDIs] Don't allocate one DPMarker per instruction (#79345)

jmorse · jmorse · commit ddc493579fa1 · 2024-02-06T13:29:56.000Z
This is an optimisation patch that shouldn't have any functional effect.
There's no need for all instructions to have a DPMarker attached to them,
because not all instructions have adjacent DPValues (aka dbg.values).

This patch inserts the appropriate conditionals into functions like
BasicBlock::spliceDebugInfo to ensure we don't step on a null pointer when
there isn't a DPMarker allocated. Mostly, this is a case of calling
createMarker occasionally, which will create a marker on an instruction
if there isn't one there already.

Also folded into this is the use of adoptDbgValues, which is a natural
extension: if we have a sequence of instructions and debug records:

    %foo = add i32 %0,...
    # dbg_value { %foo, ...
    # dbg_value { %bar, ...
    %baz = add i32 %...
    %qux = add i32 %...

and delete, for example, the %baz instruction, then the dbg_value records
would naturally be transferred onto the %qux instruction (they "fall down"
onto it). There's no point in creating and splicing DPMarkers in the case
shown when %qux doesn't have a DPMarker already, we can instead just change
the owner of %baz's DPMarker from %baz to %qux. This also avoids calling
setParent on every DPValue.

Update LoopRotationUtils: it was relying on each instruction having it's
own distinct end(), so that we could express ranges and lack-of-ranges.
That's no longer true though: so switch to storing the range of DPValues on
the next instruction when we want to consider it's range next time around
the loop (see the nearby comment).
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
@@ -90,6 +90,12 @@ class Instruction : public User,
   /// Returns true if any DPValues are attached to this instruction.
   bool hasDbgValues() const;
 
+  /// Transfer any DPValues on the position \p It onto this instruction,
+  /// by simply adopting the sequence of DPValues (which is efficient) if
+  /// possible, by merging two sequences otherwise.
+  void adoptDbgValues(BasicBlock *BB, InstListType::iterator It,
+                      bool InsertAtHead);
+
   /// Erase any DPValues attached to this instruction.
   void dropDbgValues();
 
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
@@ -81,8 +81,10 @@ void BasicBlock::convertToNewDbgValues() {
       continue;
     }
 
-    // Create a marker to store DPValues in. Technically we don't need to store
-    // one marker per instruction, but that's a future optimisation.
+    if (DPVals.empty())
+      continue;
+
+    // Create a marker to store DPValues in.
     createMarker(&I);
     DPMarker *Marker = I.DbgMarker;
 
@@ -769,6 +771,7 @@ void BasicBlock::flushTerminatorDbgValues() {
     return;
 
   // Transfer DPValues from the trailing position onto the terminator.
+  createMarker(Term);
   Term->DbgMarker->absorbDebugValues(*TrailingDPValues, false);
   TrailingDPValues->eraseFromParent();
   deleteTrailingDPValues();
@@ -812,10 +815,9 @@ void BasicBlock::spliceDebugInfoEmptyBlock(BasicBlock::iterator Dest,
     if (!SrcTrailingDPValues)
       return;
 
-    DPMarker *M = Dest->DbgMarker;
-    M->absorbDebugValues(*SrcTrailingDPValues, InsertAtHead);
-    SrcTrailingDPValues->eraseFromParent();
-    Src->deleteTrailingDPValues();
+    Dest->adoptDbgValues(Src, Src->end(), InsertAtHead);
+    // adoptDbgValues should have released the trailing DPValues.
+    assert(!Src->getTrailingDPValues());
     return;
   }
 
@@ -882,16 +884,14 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
     }
 
     if (First->hasDbgValues()) {
-      DPMarker *CurMarker = Src->getMarker(First);
       // Place them at the front, it would look like this:
       //            Dest
       //              |
       // this-block:
       // Src-block: ~~~~~~~~++++B---B---B---B:::C
       //                        |               |
       //                       First           Last
-      CurMarker->absorbDebugValues(*OurTrailingDPValues, true);
-      OurTrailingDPValues->eraseFromParent();
+      First->adoptDbgValues(this, end(), true);
     } else {
       // No current marker, create one and absorb in. (FIXME: we can avoid an
       // allocation in the future).
@@ -911,7 +911,8 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
   if (!MoreDanglingDPValues)
     return;
 
-  // FIXME: we could avoid an allocation here sometimes.
+  // FIXME: we could avoid an allocation here sometimes. (adoptDbgValues
+  // requires an iterator).
   DPMarker *LastMarker = Src->createMarker(Last);
   LastMarker->absorbDebugValues(*MoreDanglingDPValues, true);
   MoreDanglingDPValues->eraseFromParent();
@@ -993,44 +994,50 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
   // Detach the marker at Dest -- this lets us move the "====" DPValues around.
   DPMarker *DestMarker = nullptr;
   if (Dest != end()) {
-    DestMarker = getMarker(Dest);
-    DestMarker->removeFromParent();
-    createMarker(&*Dest);
+    if ((DestMarker = getMarker(Dest)))
+      DestMarker->removeFromParent();
   }
 
   // If we're moving the tail range of DPValues (":::"), absorb them into the
   // front of the DPValues at Dest.
   if (ReadFromTail && Src->getMarker(Last)) {
-    DPMarker *OntoDest = getMarker(Dest);
     DPMarker *FromLast = Src->getMarker(Last);
-    OntoDest->absorbDebugValues(*FromLast, true);
     if (LastIsEnd) {
-      FromLast->eraseFromParent();
-      Src->deleteTrailingDPValues();
+      Dest->adoptDbgValues(Src, Last, true);
+      // adoptDbgValues will release any trailers.
+      assert(!Src->getTrailingDPValues());
+    } else {
+      // FIXME: can we use adoptDbgValues here to reduce allocations?
+      DPMarker *OntoDest = createMarker(Dest);
+      OntoDest->absorbDebugValues(*FromLast, true);
     }
   }
 
   // If we're _not_ reading from the head of First, i.e. the "++++" DPValues,
   // move their markers onto Last. They remain in the Src block. No action
   // needed.
   if (!ReadFromHead && First->hasDbgValues()) {
-    DPMarker *OntoLast = Src->createMarker(Last);
-    DPMarker *FromFirst = Src->createMarker(First);
-    OntoLast->absorbDebugValues(*FromFirst,
-                                true); // Always insert at head of it.
+    if (Last != Src->end()) {
+      Last->adoptDbgValues(Src, First, true);
+    } else {
+      DPMarker *OntoLast = Src->createMarker(Last);
+      DPMarker *FromFirst = Src->createMarker(First);
+      // Always insert at front of Last.
+      OntoLast->absorbDebugValues(*FromFirst, true);
+    }
   }
 
   // Finally, do something with the "====" DPValues we detached.
   if (DestMarker) {
     if (InsertAtHead) {
       // Insert them at the end of the DPValues at Dest. The "::::" DPValues
       // might be in front of them.
-      DPMarker *NewDestMarker = getMarker(Dest);
+      DPMarker *NewDestMarker = createMarker(Dest);
       NewDestMarker->absorbDebugValues(*DestMarker, false);
     } else {
       // Insert them right at the start of the range we moved, ahead of First
       // and the "++++" DPValues.
-      DPMarker *FirstMarker = getMarker(First);
+      DPMarker *FirstMarker = createMarker(First);
       FirstMarker->absorbDebugValues(*DestMarker, true);
     }
     DestMarker->eraseFromParent();
@@ -1082,9 +1089,7 @@ void BasicBlock::insertDPValueAfter(DPValue *DPV, Instruction *I) {
   assert(I->getParent() == this);
 
   iterator NextIt = std::next(I->getIterator());
-  DPMarker *NextMarker = getMarker(NextIt);
-  if (!NextMarker)
-    NextMarker = createMarker(NextIt);
+  DPMarker *NextMarker = createMarker(NextIt);
   NextMarker->insertDPValue(DPV, true);
 }
 
@@ -1097,6 +1102,7 @@ void BasicBlock::insertDPValueBefore(DPValue *DPV,
   if (!Where->DbgMarker)
     createMarker(Where);
   bool InsertAtHead = Where.getHeadBit();
+  createMarker(&*Where);
   Where->DbgMarker->insertDPValue(DPV, InsertAtHead);
 }
 
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -434,13 +434,23 @@ void DPMarker::removeMarker() {
   // instruction. If there isn't a next instruction, put them on the
   // "trailing" list.
   DPMarker *NextMarker = Owner->getParent()->getNextMarker(Owner);
-  if (NextMarker == nullptr) {
-    NextMarker = new DPMarker();
-    Owner->getParent()->setTrailingDPValues(NextMarker);
+  if (NextMarker) {
+    NextMarker->absorbDebugValues(*this, true);
+    eraseFromParent();
+  } else {
+    // We can avoid a deallocation -- just store this marker onto the next
+    // instruction. Unless we're at the end of the block, in which case this
+    // marker becomes the trailing marker of a degenerate block.
+    BasicBlock::iterator NextIt = std::next(Owner->getIterator());
+    if (NextIt == getParent()->end()) {
+      getParent()->setTrailingDPValues(this);
+      MarkedInstr = nullptr;
+    } else {
+      NextIt->DbgMarker = this;
+      MarkedInstr = &*NextIt;
+    }
   }
-  NextMarker->absorbDebugValues(*this, true);
-
-  eraseFromParent();
+  Owner->DbgMarker = nullptr;
 }
 
 void DPMarker::removeFromParent() {
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
@@ -111,11 +111,6 @@ void Instruction::insertAfter(Instruction *InsertPos) {
   BasicBlock *DestParent = InsertPos->getParent();
 
   DestParent->getInstList().insertAfter(InsertPos->getIterator(), this);
-
-  // No need to manually update DPValues: if we insert after an instruction
-  // position, then we can never have any DPValues on "this".
-  if (DestParent->IsNewDbgInfoFormat)
-    DestParent->createMarker(this);
 }
 
 BasicBlock::iterator Instruction::insertInto(BasicBlock *ParentBB,
@@ -138,17 +133,15 @@ void Instruction::insertBefore(BasicBlock &BB,
   if (!BB.IsNewDbgInfoFormat)
     return;
 
-  BB.createMarker(this);
-
   // We've inserted "this": if InsertAtHead is set then it comes before any
   // DPValues attached to InsertPos. But if it's not set, then any DPValues
   // should now come before "this".
   bool InsertAtHead = InsertPos.getHeadBit();
   if (!InsertAtHead) {
     DPMarker *SrcMarker = BB.getMarker(InsertPos);
-    // If there's no source marker, InsertPos is very likely end().
-    if (SrcMarker)
-      DbgMarker->absorbDebugValues(*SrcMarker, false);
+    if (SrcMarker && !SrcMarker->empty()) {
+      adoptDbgValues(&BB, InsertPos, false);
+    }
   }
 
   // If we're inserting a terminator, check if we need to flush out
@@ -212,14 +205,13 @@ void Instruction::moveBeforeImpl(BasicBlock &BB, InstListType::iterator I,
   BB.getInstList().splice(I, getParent()->getInstList(), getIterator());
 
   if (BB.IsNewDbgInfoFormat && !Preserve) {
-    if (!DbgMarker)
-      BB.createMarker(this);
     DPMarker *NextMarker = getParent()->getNextMarker(this);
 
     // If we're inserting at point I, and not in front of the DPValues attached
     // there, then we should absorb the DPValues attached to I.
-    if (NextMarker && !InsertAtHead)
-      DbgMarker->absorbDebugValues(*NextMarker, false);
+    if (!InsertAtHead && NextMarker && !NextMarker->empty()) {
+      adoptDbgValues(&BB, I, false);
+    }
   }
 
   if (isTerminator())
@@ -258,6 +250,48 @@ std::optional<DPValue::self_iterator> Instruction::getDbgReinsertionPosition() {
 
 bool Instruction::hasDbgValues() const { return !getDbgValueRange().empty(); }
 
+void Instruction::adoptDbgValues(BasicBlock *BB, BasicBlock::iterator It,
+                                 bool InsertAtHead) {
+  DPMarker *SrcMarker = BB->getMarker(It);
+  auto ReleaseTrailingDPValues = [BB, It, SrcMarker]() {
+    if (BB->end() == It) {
+      SrcMarker->eraseFromParent();
+      BB->deleteTrailingDPValues();
+    }
+  };
+
+  if (!SrcMarker || SrcMarker->StoredDPValues.empty()) {
+    ReleaseTrailingDPValues();
+    return;
+  }
+
+  // If we have DPMarkers attached to this instruction, we have to honour the
+  // ordering of DPValues between this and the other marker. Fall back to just
+  // absorbing from the source.
+  if (DbgMarker || It == BB->end()) {
+    // Ensure we _do_ have a marker.
+    getParent()->createMarker(this);
+    DbgMarker->absorbDebugValues(*SrcMarker, InsertAtHead);
+
+    // Having transferred everything out of SrcMarker, we _could_ clean it up
+    // and free the marker now. However, that's a lot of heap-accounting for a
+    // small amount of memory with a good chance of re-use. Leave it for the
+    // moment. It will be released when the Instruction is freed in the worst
+    // case.
+    // However: if we transferred from a trailing marker off the end of the
+    // block, it's important to not leave the empty marker trailing. It will
+    // give a misleading impression that some debug records have been left
+    // trailing.
+    ReleaseTrailingDPValues();
+  } else {
+    // Optimisation: we're transferring all the DPValues from the source marker
+    // onto this empty location: just adopt the other instructions marker.
+    DbgMarker = SrcMarker;
+    DbgMarker->MarkedInstr = this;
+    It->DbgMarker = nullptr;
+  }
+}
+
 void Instruction::dropDbgValues() {
   if (DbgMarker)
     DbgMarker->dropDPValues();
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
@@ -2044,7 +2044,7 @@ static void insertDPValuesForPHIs(BasicBlock *BB,
     auto InsertionPt = Parent->getFirstInsertionPt();
     assert(InsertionPt != Parent->end() && "Ill-formed basic block");
 
-    InsertionPt->DbgMarker->insertDPValue(NewDbgII, true);
+    Parent->insertDPValueBefore(NewDbgII, InsertionPt);
   }
 }
 
diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
@@ -596,7 +596,10 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
     // on the next instruction, here labelled xyzzy, before we hoist %foo.
     // Later, we only only clone DPValues from that position (xyzzy) onwards,
     // which avoids cloning DPValue "blah" multiple times.
-    std::optional<DPValue::self_iterator> NextDbgInst = std::nullopt;
+    // (Stored as a range because it gives us a natural way of testing whether
+    //  there were DPValues on the next instruction before we hoisted things).
+    iterator_range<DPValue::self_iterator> NextDbgInsts =
+      (I != E) ? I->getDbgValueRange() : DPMarker::getEmptyDPValueRange();
 
     while (I != E) {
       Instruction *Inst = &*I++;
@@ -611,9 +614,10 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
           !Inst->mayWriteToMemory() && !Inst->isTerminator() &&
           !isa<DbgInfoIntrinsic>(Inst) && !isa<AllocaInst>(Inst)) {
 
-        if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat) {
+        if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat &&
+            !NextDbgInsts.empty()) {
           auto DbgValueRange =
-              LoopEntryBranch->cloneDebugInfoFrom(Inst, NextDbgInst);
+              LoopEntryBranch->cloneDebugInfoFrom(Inst, NextDbgInsts.begin());
           RemapDPValueRange(M, DbgValueRange, ValueMap,
                             RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
           // Erase anything we've seen before.
@@ -622,7 +626,8 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
               DPV.eraseFromParent();
         }
 
-        NextDbgInst = I->getDbgValueRange().begin();
+        NextDbgInsts = I->getDbgValueRange();
+
         Inst->moveBefore(LoopEntryBranch);
 
         ++NumInstrsHoisted;
@@ -635,11 +640,12 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
 
       ++NumInstrsDuplicated;
 
-      if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat) {
-        auto Range = C->cloneDebugInfoFrom(Inst, NextDbgInst);
+      if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat &&
+          !NextDbgInsts.empty()) {
+        auto Range = C->cloneDebugInfoFrom(Inst, NextDbgInsts.begin());
         RemapDPValueRange(M, Range, ValueMap,
                           RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
-        NextDbgInst = std::nullopt;
+        NextDbgInsts = DPMarker::getEmptyDPValueRange();
         // Erase anything we've seen before.
         for (DPValue &DPV : make_early_inc_range(Range))
           if (DbgIntrinsics.count(makeHash(&DPV)))
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -225,10 +225,9 @@ TEST(BasicBlockDbgInfoTest, MarkerOperations) {
   // then they would sit "above" the new instruction.
   Instr1->insertBefore(BB, BB.end());
   EXPECT_EQ(Instr1->DbgMarker->StoredDPValues.size(), 2u);
-  // However we won't de-allocate the trailing marker until a terminator is
-  // inserted.
-  EXPECT_EQ(EndMarker->StoredDPValues.size(), 0u);
-  EXPECT_EQ(BB.getTrailingDPValues(), EndMarker);
+  // We should de-allocate the trailing marker when something is inserted
+  // at end().
+  EXPECT_EQ(BB.getTrailingDPValues(), nullptr);
 
   // Remove Instr1: now the DPValues will fall down again,
   Instr1->removeFromParent();
@@ -394,12 +393,12 @@ TEST(BasicBlockDbgInfoTest, InstrDbgAccess) {
   Instruction *CInst = BInst->getNextNode();
   Instruction *DInst = CInst->getNextNode();
 
-  ASSERT_TRUE(BInst->DbgMarker);
+  ASSERT_FALSE(BInst->DbgMarker);
   ASSERT_TRUE(CInst->DbgMarker);
   ASSERT_EQ(CInst->DbgMarker->StoredDPValues.size(), 1u);
   DPValue *DPV1 = &*CInst->DbgMarker->StoredDPValues.begin();
   ASSERT_TRUE(DPV1);
-  EXPECT_EQ(BInst->DbgMarker->StoredDPValues.size(), 0u);
+  EXPECT_FALSE(BInst->hasDbgValues());
 
   // Clone DPValues from one inst to another. Other arguments to clone are
   // tested in DPMarker test.

Original file line number	Diff line number	Diff line change
`@@ -2044,7 +2044,7 @@ static void insertDPValuesForPHIs(BasicBlock *BB,`
`2044`	`2044`	`auto InsertionPt = Parent->getFirstInsertionPt();`
`2045`	`2045`	`assert(InsertionPt != Parent->end() && "Ill-formed basic block");`
`2046`	`2046`
`2047`		`- InsertionPt->DbgMarker->insertDPValue(NewDbgII, true);`
	`2047`	`+ Parent->insertDPValueBefore(NewDbgII, InsertionPt);`
`2048`	`2048`	`}`
`2049`	`2049`	`}`
`2050`	`2050`