[clang-tools-extra] [llvm] [clang] [DebugInfo][RemoveDIs] Don't allocate one DPMarker per instruction (PR #79345)
Jeremy Morse via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 5 09:39:48 PST 2024
https://github.com/jmorse updated https://github.com/llvm/llvm-project/pull/79345
>From 0620bf77b4a8586dc1aa96d72bd088a3e601edd4 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Wed, 24 Jan 2024 16:33:16 +0000
Subject: [PATCH 1/4] [DebugInfo][RemoveDIs] Don't allocate one DPMarker per
instruction
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).
---
llvm/include/llvm/IR/Instruction.h | 5 ++
llvm/lib/IR/BasicBlock.cpp | 59 +++++++++++--------
llvm/lib/IR/DebugProgramInstruction.cpp | 22 +++++--
llvm/lib/IR/Instruction.cpp | 47 ++++++++++-----
llvm/lib/Transforms/Utils/Local.cpp | 2 +-
.../Transforms/Utils/LoopRotationUtils.cpp | 20 ++++---
llvm/unittests/IR/BasicBlockDbgInfoTest.cpp | 11 ++--
7 files changed, 108 insertions(+), 58 deletions(-)
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index fcd2ba838e7fd..d120ef603bafb 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 dca5283283847..c7b1bd9c3ede2 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 fd234685d5fd4..8b7761f8de161 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 d7bf1447921fe..0f270199b1318 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 459e3d9805928..e4aa25f7ac6ad 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 504f4430dc2ca..ec59a07730203 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 fb4847fc0a826..827b4a9c0cc32 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.
>From 7c172dfa4b8f243a6ab9f3275a312806c2d7a014 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at gmail.com>
Date: Fri, 26 Jan 2024 14:32:24 +0000
Subject: [PATCH 2/4] Rename comment function
Co-authored-by: Stephen Tozer <Melamoto at gmail.com>
---
llvm/lib/IR/BasicBlock.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index c7b1bd9c3ede2..75e640d0426a1 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -914,7 +914,7 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
if (!MoreDanglingDPValues)
return;
- // FIXME: we could avoid an allocation here sometimes. (absorbDbgValues
+ // FIXME: we could avoid an allocation here sometimes. (adoptDbgValues
// requires an iterator).
DPMarker *LastMarker = Src->createMarker(Last);
LastMarker->absorbDebugValues(*MoreDanglingDPValues, true);
>From ad1bc4edd4d595051bd3a961cb9a9d83719d6087 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Fri, 26 Jan 2024 14:51:15 +0000
Subject: [PATCH 3/4] Reword a comment
---
llvm/include/llvm/IR/Instruction.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index d120ef603bafb..70009808075e3 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -86,8 +86,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.
- /// \returns true if adoption worked, false otherwise.
+ /// 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.
+ /// \returns true if adoption worked, false otherwise. The source sequence
+ /// of DPValues may need to be erased if adoption fails.
+ [[nodiscard]]
bool adoptDbgValues(BasicBlock *BB, InstListType::iterator It,
bool InsertAtHead);
>From 3e4d83e21e8e8a9249c5901b31ce3142d08f88b7 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Mon, 5 Feb 2024 17:44:49 +0000
Subject: [PATCH 4/4] Always clean-up in adoptDbgValues
---
llvm/include/llvm/IR/Instruction.h | 5 +----
llvm/lib/IR/BasicBlock.cpp | 20 ++++++++----------
llvm/lib/IR/Instruction.cpp | 33 ++++++++++++++++++++++--------
3 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 116b74b1be1c3..cd814e21be1ae 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -93,10 +93,7 @@ class Instruction : public User,
/// 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.
- /// \returns true if adoption worked, false otherwise. The source sequence
- /// of DPValues may need to be erased if adoption fails.
- [[nodiscard]]
- bool adoptDbgValues(BasicBlock *BB, InstListType::iterator It,
+ void adoptDbgValues(BasicBlock *BB, InstListType::iterator It,
bool InsertAtHead);
/// Erase any DPValues attached to this instruction.
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 75e640d0426a1..358d6fd26cf10 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -815,11 +815,9 @@ void BasicBlock::spliceDebugInfoEmptyBlock(BasicBlock::iterator Dest,
if (!SrcTrailingDPValues)
return;
- 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();
+ Dest->adoptDbgValues(Src, Src->end(), InsertAtHead);
+ // adoptDbgValues should have released the trailing DPValues.
+ assert(!Src->getTrailingDPValues());
return;
}
@@ -893,8 +891,7 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src,
// Src-block: ~~~~~~~~++++B---B---B---B:::C
// | |
// First Last
- if (!First->adoptDbgValues(this, end(), 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).
@@ -1006,9 +1003,9 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
if (ReadFromTail && Src->getMarker(Last)) {
DPMarker *FromLast = Src->getMarker(Last);
if (LastIsEnd) {
- if (!Dest->adoptDbgValues(Src, Last, true))
- 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);
@@ -1022,8 +1019,7 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src,
if (!ReadFromHead && First->hasDbgValues()) {
DPMarker *FromFirst = Src->getMarker(First);
if (Last != Src->end()) {
- if (!Last->adoptDbgValues(Src, First, true))
- FromFirst->eraseFromParent();
+ Last->adoptDbgValues(Src, First, true);
} else {
DPMarker *OntoLast = Src->createMarker(Last);
DPMarker *FromFirst = Src->createMarker(First);
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 60e38c6c4b8d3..23a3e72da51d4 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -140,10 +140,7 @@ void Instruction::insertBefore(BasicBlock &BB,
if (!InsertAtHead) {
DPMarker *SrcMarker = BB.getMarker(InsertPos);
if (SrcMarker && !SrcMarker->empty()) {
- if (!adoptDbgValues(&BB, InsertPos, false))
- SrcMarker->eraseFromParent();
- if (InsertPos == BB.end())
- BB.deleteTrailingDPValues();
+ adoptDbgValues(&BB, InsertPos, false);
}
}
@@ -253,11 +250,20 @@ std::optional<DPValue::self_iterator> Instruction::getDbgReinsertionPosition() {
bool Instruction::hasDbgValues() const { return !getDbgValueRange().empty(); }
-bool Instruction::adoptDbgValues(BasicBlock *BB, BasicBlock::iterator It,
+void Instruction::adoptDbgValues(BasicBlock *BB, BasicBlock::iterator It,
bool InsertAtHead) {
DPMarker *SrcMarker = BB->getMarker(It);
- if (!SrcMarker || SrcMarker->StoredDPValues.empty())
- return false;
+ 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
@@ -266,14 +272,23 @@ bool Instruction::adoptDbgValues(BasicBlock *BB, BasicBlock::iterator It,
// Ensure we _do_ have a marker.
getParent()->createMarker(this);
DbgMarker->absorbDebugValues(*SrcMarker, InsertAtHead);
- return false;
+
+ // 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;
- return true;
}
}
More information about the cfe-commits
mailing list