[DebugInfo][RemoveDIs] Don't allocate one DPMarker per instruction #79345
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesHere'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: 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:
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.
|
You can test this locally with the following command:git-clang-format --diff 702664e7870c27f197dfb744a4db54aa259ce452 3e4d83e21e8e8a9249c5901b31ce3142d08f88b7 -- 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.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
index ec59a07730..be10ee5165 100644
--- a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
@@ -599,7 +599,7 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
// (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();
+ (I != E) ? I->getDbgValueRange() : DPMarker::getEmptyDPValueRange();
while (I != E) {
Instruction *Inst = &*I++;
|
SLTozer
approved these changes
Jan 26, 2024
There was a problem hiding this comment.
Looks broadly good - though I feel like (YMMV) there are a lot of different places where we're doing mostly similar but slightly different things with debug ranges, and it's not easy to intuit what's happening in each case and what the difference is without really digging into it; I reckon it might be worth looking to see if there's more that can be done to extract common behaviour out to other functions, but that's a separate patch imo since this patch has a clear functional benefit.
| @@ -882,16 +886,15 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src, | |||
| } | |||
|
|
|||
| if (First->hasDbgValues()) { | |||
There was a problem hiding this comment.
Can't this whole if/else block be replaced by just adoptDbgValues?
OCHyams
reviewed
Jan 26, 2024
llvm/include/llvm/IR/Instruction.h
Outdated
| @@ -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, | |||
There was a problem hiding this comment.
Is there a way to word this where the method of moving is more explicitly called out (plus consequences of returning true/false, if it's easy to summarise)?
| if (NextMarker && !InsertAtHead) | ||
| DbgMarker->absorbDebugValues(*NextMarker, false); | ||
| if (!InsertAtHead && NextMarker && !NextMarker->empty()) { | ||
| adoptDbgValues(&BB, I, false); |
There was a problem hiding this comment.
Given that (most? all?) other uses of adoptDbgValues involve a check-if-not-then-cleanup-something, can this get a comment explaining why it isn't needed?
Do we want to make adoptDbgValues nodiscard, or is that too severe?
+1 to what @SLTozer said:
it's not easy to intuit what's happening in each case and what the difference is without really digging into it ... [can we] extract common behaviour out to other functions [in a future patch].
There was a problem hiding this comment.
Hmmm, I was going to say "because we never have trailing DPValues here" (i.e. ones hanging off the end of the block), but I'm now seeing various scenarios where moveBefore is called with the end() iterator. And the assertion on moveBeforeImpl's entry explicitly takes account of that. So this would need some cleanup.
It's probably better to fold all that maintenance code into adoptDbgValues, I suppose I'll look at that next.
|
On the simplification front, perhaps things could be refactored into some ideas of swapping-collections between locations (like the towers of Hanoi...), the difficulty then is that we concatenate collections of DPValues together at some points, and not at others, and not always in the same order. I don't think there's a really simple way of representing this: we've effectively pushed the numerous ways you can splice instruction ranges containing dbg.values into some state (the iterator bits), and this function then has to account for the different combinations of inputs. To be clear, I don't like nastily complicated functions, but we've centralised the nastiness of debug intrinsic maintenence down into this one function, so it's not too much of a surprise that it's unpleasent. |
Co-authored-by: Stephen Tozer <Melamoto@gmail.com>
|
(Note that I've merged main into this branch for various reasons,) I've adjusted (I suppose this is a classic case of premature optimisation) |
SLTozer
approved these changes
Feb 6, 2024
jmorse
added a commit
that referenced
this pull request
Feb 6, 2024
…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).
|
Landed in ddc4935 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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).