[llvm] ddc4935 - [DebugInfo][RemoveDIs] Don't allocate one DPMarker per instruction (#79345)
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 6 05:25:11 PST 2024
Author: Jeremy Morse
Date: 2024-02-06T13:29:56Z
New Revision: ddc493579fa1b7eed058c7ed8a5a6b5755a31953
URL: https://github.com/llvm/llvm-project/commit/ddc493579fa1b7eed058c7ed8a5a6b5755a31953
DIFF: https://github.com/llvm/llvm-project/commit/ddc493579fa1b7eed058c7ed8a5a6b5755a31953.diff
LOG: [DebugInfo][RemoveDIs] Don't allocate one DPMarker per instruction (#79345)
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).
Added:
Modified:
llvm/include/llvm/IR/Instruction.h
llvm/lib/IR/BasicBlock.cpp
llvm/lib/IR/DebugProgramInstruction.cpp
llvm/lib/IR/Instruction.cpp
llvm/lib/Transforms/Utils/Local.cpp
llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index a785a43a0f11a6..cd814e21be1ae1 100644
--- 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
index dca52832838470..bb55f48df4b314 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,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,7 +884,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 +891,7 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
// 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,20 +994,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();
- 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);
}
}
@@ -1014,10 +1017,14 @@ 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.
+ 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.
@@ -1025,12 +1032,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 +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
index 03085b3bfd1279..1a62902115be1a 100644
--- 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
index 904ce17fb0e7c9..23a3e72da51d44 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,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
index 459e3d98059283..e4aa25f7ac6ad3 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 504f4430dc2ca6..ec59a077302037 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 fb4847fc0a8262..827b4a9c0cc323 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.
More information about the llvm-commits
mailing list