[llvm] [DebugInfo][RemoveDIs] Don't allocate one DPMarker per instruction (PR #79345)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 24 10:39:17 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
Author: Jeremy Morse (jmorse)
<details>
<summary>Changes</summary>
Here's a patch that avoids allocating a DPMarker for every instruction in an LLVM-IR program. This was previously providing nice properties for development, however we need to stop doing that on non-debug builds to turn this on by default (or pay a large compile-time penalty). It's easier to make all our code cope with absent DPMarkers at the same time rather than to try and separate debug-builds and non-debug builds.
(I was rather hoping we could drop this in later, but it's better to avoid the compile-time regression IMO).
Commit message follows:
~
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).
---
Full diff: https://github.com/llvm/llvm-project/pull/79345.diff
7 Files Affected:
- (modified) llvm/include/llvm/IR/Instruction.h (+5)
- (modified) llvm/lib/IR/BasicBlock.cpp (+35-24)
- (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+16-6)
- (modified) llvm/lib/IR/Instruction.cpp (+33-14)
- (modified) llvm/lib/Transforms/Utils/Local.cpp (+1-1)
- (modified) llvm/lib/Transforms/Utils/LoopRotationUtils.cpp (+13-7)
- (modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (+5-6)
``````````diff
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index fcd2ba838e7fd51..d120ef603bafbd9 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -86,6 +86,11 @@ 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.
+ /// \returns true if adoption worked, false otherwise.
+ bool 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
index dca528328384701..c7b1bd9c3ede289 100644
--- 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,9 +815,10 @@ void BasicBlock::spliceDebugInfoEmptyBlock(BasicBlock::iterator Dest,
if (!SrcTrailingDPValues)
return;
- DPMarker *M = Dest->DbgMarker;
- M->absorbDebugValues(*SrcTrailingDPValues, InsertAtHead);
- SrcTrailingDPValues->eraseFromParent();
+ if (!Dest->adoptDbgValues(Src, Src->end(), InsertAtHead))
+ // If we couldn't just assign the marker into Dest, delete the trailing
+ // DPMarker.
+ SrcTrailingDPValues->eraseFromParent();
Src->deleteTrailingDPValues();
return;
}
@@ -882,7 +886,6 @@ 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
// |
@@ -890,8 +893,8 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
// Src-block: ~~~~~~~~++++B---B---B---B:::C
// | |
// First Last
- CurMarker->absorbDebugValues(*OurTrailingDPValues, true);
- OurTrailingDPValues->eraseFromParent();
+ if (!First->adoptDbgValues(this, end(), true))
+ OurTrailingDPValues->eraseFromParent();
} else {
// No current marker, create one and absorb in. (FIXME: we can avoid an
// allocation in the future).
@@ -911,7 +914,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. (absorbDbgValues
+ // requires an iterator).
DPMarker *LastMarker = Src->createMarker(Last);
LastMarker->absorbDebugValues(*MoreDanglingDPValues, true);
MoreDanglingDPValues->eraseFromParent();
@@ -993,20 +997,22 @@ 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();
+ if (!Dest->adoptDbgValues(Src, Last, true))
+ FromLast->eraseFromParent();
Src->deleteTrailingDPValues();
+ } else {
+ // FIXME: can we use adoptDbgValues here to reduce allocations?
+ DPMarker *OntoDest = createMarker(Dest);
+ OntoDest->absorbDebugValues(*FromLast, true);
}
}
@@ -1014,10 +1020,16 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
// 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.
+ DPMarker *FromFirst = Src->getMarker(First);
+ if (Last != Src->end()) {
+ if (!Last->adoptDbgValues(Src, First, true))
+ FromFirst->eraseFromParent();
+ } 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.
@@ -1025,12 +1037,12 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
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 +1094,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 +1107,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
index fd234685d5fd4bd..8b7761f8de161a1 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -430,13 +430,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
index d7bf1447921fec7..0f270199b131819 100644
--- 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,18 @@ 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()) {
+ if (!adoptDbgValues(&BB, InsertPos, false))
+ SrcMarker->eraseFromParent();
+ if (InsertPos == BB.end())
+ BB.deleteTrailingDPValues();
+ }
}
// If we're inserting a terminator, check if we need to flush out
@@ -212,14 +208,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())
@@ -270,6 +265,30 @@ std::optional<DPValue::self_iterator> Instruction::getDbgReinsertionPosition() {
bool Instruction::hasDbgValues() const { return !getDbgValueRange().empty(); }
+bool Instruction::adoptDbgValues(BasicBlock *BB, BasicBlock::iterator It,
+ bool InsertAtHead) {
+ DPMarker *SrcMarker = BB->getMarker(It);
+ if (!SrcMarker || SrcMarker->StoredDPValues.empty())
+ return false;
+
+ // 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);
+ return false;
+ } 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;
+ return true;
+ }
+}
+
void Instruction::dropDbgValues() {
if (DbgMarker)
DbgMarker->dropDPValues();
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 459e3d980592833..e4aa25f7ac6ad31 100644
--- 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
index 504f4430dc2ca66..ec59a0773020374 100644
--- 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
index fb4847fc0a82626..827b4a9c0cc3233 100644
--- 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.
``````````
</details>
https://github.com/llvm/llvm-project/pull/79345
More information about the llvm-commits
mailing list