[llvm] [DebugInfo][RemoveDIs] Emulate inserting insts in dbg.value sequences (PR #73350)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 24 08:57:50 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
Author: Jeremy Morse (jmorse)
<details>
<summary>Changes</summary>
Here's a problem for the RemoveDIs project to make debug-info not be stored in instructions -- in the following sequence:
dbg.value(foo
%bar = add i32 ...
dbg.value(baz
It's possible for rare passes (only CodeGenPrepare) to remove the add instruction, and then re-insert it back in the same place. When debug-info is stored in instructions and there's a total order on "when" things happen this is easy, but by moving that information out of the instruction stream we start having to do manual maintenance.
This patch adds some utilities for re-inserting an instruction into a sequence of DPValue objects. Someday we hope to design this away, but for now it's necessary to support all the things you can do with dbg.values. The two unit tests show how DPValues get shuffled around using the relevant function calls. A follow-up patch adds instrumentation to CodeGenPrepare.
---
Full diff: https://github.com/llvm/llvm-project/pull/73350.diff
5 Files Affected:
- (modified) llvm/include/llvm/IR/BasicBlock.h (+8)
- (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+10)
- (modified) llvm/lib/IR/BasicBlock.cpp (+49)
- (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+25)
- (modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (+144)
``````````diff
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index ec916acc25151c8..45e4b577c8a992d 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -141,6 +141,14 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
/// move such DPValues back to the right place (ahead of the terminator).
void flushTerminatorDbgValues();
+ /// In rare circumstances instructions can be speculatively removed from
+ /// blocks, and then be re-inserted back into that position later. When this
+ /// happens in RemoveDIs debug-info mode, some special patching-up needs to
+ /// occur: inserting into the middle of a sequence of dbg.value intrinsics
+ /// does not have an equivalent with DPValues.
+ void reinsertInstInDPValues(Instruction *I,
+ std::optional<DPValue::self_iterator> Pos);
+
private:
void setParent(Function *parent);
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index cfee2a87b75c7b5..93adf866054de4f 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -308,6 +308,11 @@ class DPMarker {
/// Transfer any DPValues from \p Src into this DPMarker. If \p InsertAtHead
/// is true, place them before existing DPValues, otherwise afterwards.
void absorbDebugValues(DPMarker &Src, bool InsertAtHead);
+ /// Transfer the DPValues in \p Range from \p Src into this DPMarker. If
+ /// \p InsertAtHead is true, place them before existing DPValues, otherwise
+ // afterwards.
+ void absorbDebugValues(iterator_range<DPValue::self_iterator> Range,
+ DPMarker &Src, bool InsertAtHead);
/// Insert a DPValue into this DPMarker, at the end of the list. If
/// \p InsertAtHead is true, at the start.
void insertDPValue(DPValue *New, bool InsertAtHead);
@@ -328,6 +333,11 @@ class DPMarker {
/// erasing a dbg.value from a block.
void dropOneDPValue(DPValue *DPV);
+ /// Return an iterator to the position of the "Next" DPValue after this
+ /// marker, or std::nullopt. This is the position to pass to
+ /// BasicBlock::reinsertInstInDPValues when re-inserting an instruction.
+ std::optional<DPValue::self_iterator> getReinsertionPosition();
+
/// We generally act like all llvm Instructions have a range of DPValues
/// attached to them, but in reality sometimes we don't allocate the DPMarker
/// to save time and memory, but still have to return ranges of DPValues. When
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 6c08ca1efc65288..27a797438891085 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -1013,6 +1013,55 @@ DPMarker *BasicBlock::getMarker(InstListType::iterator It) {
return It->DbgMarker;
}
+void BasicBlock::reinsertInstInDPValues(
+ Instruction *I, std::optional<DPValue::self_iterator> Pos) {
+ // "I" was originally removed from a position where it was immediately in
+ // front of Pos. Any DPValues on that position then "fell down" onto Pos.
+ // "I" has been re-inserted after that wedge of DPValues, shuffle them around
+ // to represent the original positioning. To illustrate:
+ //
+ // Instructions: I1---I---I0
+ // DPValues: DDD DDD
+ //
+ // Instruction "I" removed,
+ //
+ // Instructions: I1------I0
+ // DPValues: DDDDDD
+ // ^Pos
+ //
+ // Instruction "I" re-inserted (now):
+ //
+ // Instructions: I1------I-I0
+ // DPValues: DDDDDD
+ // ^Pos
+ //
+ // After this method completes:
+ //
+ // Instructions: I1---I---I0
+ // DPValues: DDD DDD
+ //
+ // In a fantastic future we would a) ban passes from doing this at all, but
+ // in lieu of that b) we could have more fine grained control over how
+ // debug-info records coalesce together. This will probably happen if/when we
+ // address the matter of all debug-info records having to have a total order.
+
+ // If there were no DPValues on I0, Pos will be empty. We also don't need to
+ // do any further maintanence.
+ if (!Pos)
+ return;
+
+ // Construct the range of DPMarkers to move.
+ DPMarker *DPM = (*Pos)->getMarker();
+ auto Range = make_range(*Pos, DPM->StoredDPValues.end());
+ assert(Range.begin() != Range.end());
+
+ // These are DPValues that used to be attached to I0 but are now attached to I
+ // after the re-insertion. Move them back onto I0.
+ DPMarker *NextMarker = createMarker(std::next(I->getIterator()));
+ assert(NextMarker->StoredDPValues.empty());
+ NextMarker->absorbDebugValues(Range, *DPM, true);
+}
+
#ifndef NDEBUG
/// In asserts builds, this checks the numbering. In non-asserts builds, it
/// is defined as a no-op inline function in BasicBlock.h.
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 581d77a26acb80a..f0e00b0d7a40a5f 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -267,6 +267,19 @@ void DPMarker::dropOneDPValue(DPValue *DPV) {
DPV->deleteInstr();
}
+std::optional<DPValue::self_iterator> DPMarker::getReinsertionPosition() {
+ // Is there a marker on the next instruction?
+ DPMarker *NextMarker = getParent()->getNextMarker(MarkedInstr);
+ if (!NextMarker)
+ return std::nullopt;
+
+ // Are there any DPValues in the next marker?
+ if (NextMarker->StoredDPValues.empty())
+ return std::nullopt;
+
+ return NextMarker->StoredDPValues.begin();
+}
+
const BasicBlock *DPMarker::getParent() const {
return MarkedInstr->getParent();
}
@@ -334,6 +347,18 @@ void DPMarker::absorbDebugValues(DPMarker &Src, bool InsertAtHead) {
StoredDPValues.splice(It, Src.StoredDPValues);
}
+void DPMarker::absorbDebugValues(iterator_range<DPValue::self_iterator> Range,
+ DPMarker &Src, bool InsertAtHead) {
+ for (DPValue &DPV : Range)
+ DPV.setMarker(this);
+
+ auto InsertPos =
+ (InsertAtHead) ? StoredDPValues.begin() : StoredDPValues.end();
+
+ StoredDPValues.splice(InsertPos, Src.StoredDPValues, Range.begin(),
+ Range.end());
+}
+
iterator_range<simple_ilist<DPValue>::iterator> DPMarker::cloneDebugInfoFrom(
DPMarker *From, std::optional<simple_ilist<DPValue>::iterator> from_here,
bool InsertAtHead) {
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index 481cd181d3848e7..190a9d772c7889e 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -1110,5 +1110,149 @@ TEST(BasicBlockDbgInfoTest, DbgSpliceTrailing) {
UseNewDbgInfoFormat = false;
}
+// When we remove instructions from the program, adjacent DPValues coalesce
+// together into one DPMarker. In "old" dbg.value mode you could re-insert
+// the removed instruction back into the middle of a sequence of dbg.values.
+// Test that this can be replicated correctly by DPValues
+TEST(BasicBlockDbgInfoTest, RemoveInstAndReinsert) {
+ LLVMContext C;
+ UseNewDbgInfoFormat = true;
+
+ std::unique_ptr<Module> M = parseIR(C, R"(
+ define i16 @f(i16 %a) !dbg !6 {
+ entry:
+ call void @llvm.dbg.value(metadata i16 %a, metadata !9, metadata !DIExpression()), !dbg !11
+ %foo = add i16 %a, %a
+ call void @llvm.dbg.value(metadata i16 0, metadata !9, metadata !DIExpression()), !dbg !11
+ ret i16 1
+ }
+ declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+ !llvm.dbg.cu = !{!0}
+ !llvm.module.flags = !{!5}
+
+ !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+ !1 = !DIFile(filename: "t.ll", directory: "/")
+ !2 = !{}
+ !5 = !{i32 2, !"Debug Info Version", i32 3}
+ !6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+ !7 = !DISubroutineType(types: !2)
+ !8 = !{!9}
+ !9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+ !10 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_unsigned)
+ !11 = !DILocation(line: 1, column: 1, scope: !6)
+)");
+
+ BasicBlock &Entry = M->getFunction("f")->getEntryBlock();
+ M->convertToNewDbgValues();
+
+ // Fetch the relevant instructions from the converted function.
+ Instruction *AddInst = &*Entry.begin();
+ ASSERT_TRUE(isa<BinaryOperator>(AddInst));
+ Instruction *RetInst = AddInst->getNextNode();
+ ASSERT_TRUE(isa<ReturnInst>(RetInst));
+
+ // They should both have one DPValue on each.
+ EXPECT_TRUE(AddInst->hasDbgValues());
+ EXPECT_TRUE(RetInst->hasDbgValues());
+ auto R1 = AddInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R1.begin(), R1.end()), 1u);
+ auto R2 = RetInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R2.begin(), R2.end()), 1u);
+
+ // The Supported (TM) code sequence for removing then reinserting insts:
+ std::optional<DPValue::self_iterator> Pos =
+ AddInst->DbgMarker->getReinsertionPosition();
+ AddInst->removeFromParent();
+
+ // We should have a re-insertion position.
+ ASSERT_TRUE(Pos);
+ // Both DPValues should now be attached to the ret inst.
+ auto R3 = RetInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R3.begin(), R3.end()), 2u);
+
+ // Re-insert and re-insert.
+ AddInst->insertBefore(RetInst);
+ Entry.reinsertInstInDPValues(AddInst, Pos);
+ // We should be back into a position of having one DPValue on each inst.
+ EXPECT_TRUE(AddInst->hasDbgValues());
+ EXPECT_TRUE(RetInst->hasDbgValues());
+ auto R4 = AddInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R4.begin(), R4.end()), 1u);
+ auto R5 = RetInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R5.begin(), R5.end()), 1u);
+
+ UseNewDbgInfoFormat = false;
+}
+
+// Test instruction removal and re-insertion, this time with one DPValue that
+// should hop up one instruction.
+TEST(BasicBlockDbgInfoTest, RemoveInstAndReinsertForOneDPValue) {
+ LLVMContext C;
+ UseNewDbgInfoFormat = true;
+
+ std::unique_ptr<Module> M = parseIR(C, R"(
+ define i16 @f(i16 %a) !dbg !6 {
+ entry:
+ call void @llvm.dbg.value(metadata i16 %a, metadata !9, metadata !DIExpression()), !dbg !11
+ %foo = add i16 %a, %a
+ ret i16 1
+ }
+ declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+ !llvm.dbg.cu = !{!0}
+ !llvm.module.flags = !{!5}
+
+ !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+ !1 = !DIFile(filename: "t.ll", directory: "/")
+ !2 = !{}
+ !5 = !{i32 2, !"Debug Info Version", i32 3}
+ !6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+ !7 = !DISubroutineType(types: !2)
+ !8 = !{!9}
+ !9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+ !10 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_unsigned)
+ !11 = !DILocation(line: 1, column: 1, scope: !6)
+)");
+
+ BasicBlock &Entry = M->getFunction("f")->getEntryBlock();
+ M->convertToNewDbgValues();
+
+ // Fetch the relevant instructions from the converted function.
+ Instruction *AddInst = &*Entry.begin();
+ ASSERT_TRUE(isa<BinaryOperator>(AddInst));
+ Instruction *RetInst = AddInst->getNextNode();
+ ASSERT_TRUE(isa<ReturnInst>(RetInst));
+
+ // There should be one DPValue.
+ EXPECT_TRUE(AddInst->hasDbgValues());
+ EXPECT_FALSE(RetInst->hasDbgValues());
+ auto R1 = AddInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R1.begin(), R1.end()), 1u);
+
+ // The Supported (TM) code sequence for removing then reinserting insts:
+ std::optional<DPValue::self_iterator> Pos =
+ AddInst->DbgMarker->getReinsertionPosition();
+ AddInst->removeFromParent();
+
+ // No re-insertion position as there were no DPValues on the ret.
+ ASSERT_FALSE(Pos);
+ // The single DPValue should now be attached to the ret inst.
+ EXPECT_TRUE(RetInst->hasDbgValues());
+ auto R2 = RetInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R2.begin(), R2.end()), 1u);
+
+ // Re-insert and re-insert.
+ AddInst->insertBefore(RetInst);
+ Entry.reinsertInstInDPValues(AddInst, Pos);
+ // We should be back into a position of having one DPValue on the AddInst.
+ EXPECT_TRUE(AddInst->hasDbgValues());
+ EXPECT_FALSE(RetInst->hasDbgValues());
+ auto R3 = AddInst->getDbgValueRange();
+ EXPECT_EQ(std::distance(R3.begin(), R3.end()), 1u);
+
+ UseNewDbgInfoFormat = false;
+}
+
} // End anonymous namespace.
#endif // EXPERIMENTAL_DEBUGINFO_ITERATORS
``````````
</details>
https://github.com/llvm/llvm-project/pull/73350
More information about the llvm-commits
mailing list