[llvm] [DebugInfo][RemoveDIs] Add a DPValue implementation for instcombine sinking (PR #77930)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 12 05:41:41 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
Author: Jeremy Morse (jmorse)
<details>
<summary>Changes</summary>
Hi (and pre-emptive apologies for an essay),
This patch re-implements debug-info salvaging for instcombine sinking using DPValues, the replacement for dbg.value intrinsics. Unfortunately it does it in an ugly way, while I'll elaborate below. The objective here is to match what instcombine currently does, i.e. when we sink an instruction from one block to an immediate successor, the latest assignments to variables using that instruction value sink with it. I've refactor'd the dbg.value implementation into it's own function and added a DPValue one, as it's too much stuff to really observe side-by-side efficiently.
I dislike this sinking-salvaging behaviour, because it's trying to maintain an up-to-date fully-accurate varible-to-values mapping in the middle of compilation -- which is something that I don't think is feasible. As demonstrated with instruction referencing in the codegen backend, we'd be better off tolerating some use-before-defs by dbg.values and just specifying their meaning. We could technically do that right now and not sink dbg.values at all: however most architectures don't support instruction referencing yet, so they'd lose variable location coverage. Another reason I don't like this is because it's not fully accurate: debug intrinsics will re-order with intrinsics for the same variable, but which use a different LLVM-IR Value. (And fixing that would introduce a large compile-time cost I believe).
When replacing dbg.values with DPValues, we need to preserve the existing behaviour. However there's a major problem -- with dbg.value intrinsics there's a total order of instructions in a block that we can rely on for ordering dbg.values and finding the last assignment to a variable. This exists for DPValues, but it's harder to use, because the order of DPValues is hidden in a secondary list attached to each instruction, and we can't easily find the order of two DPValues that are attached to the same instruction. I've resorted to producing an instruction/Variable count-densemap to identify situations where there are duplicate sinking DPValues attached to the same instruction, then finding the last DPValue within that instruction, and recording that we should only sink that one. This is expensive, but should only fire rarely. compile time tracker:
http://llvm-compile-time-tracker.com/compare.php?from=90525125421300d9d1b6bf55288bd1871855d35d&to=eabd2a9d6922dd82b59497b769bc0a160e69c811&stat=instructions:u
And there's even more difficulty! Because we don't get and use the total order of DPValues in the block, when we clone and sink some DPValues, they won't happen in the same order as in dbg.value mode. That changes the order of creation (equivalent to the use-list order in a ValueAsMetadata), which causes other bits of LLVM to process dbg.values/DPValues in different orders. None of this causes a functional difference, but because DW_TAG_variables etc in DWARF are ordered by the order in which we encounter debug-info records in the instruction stream, we lose the ability to produce an identical binary. This is something we just have to swallow IMO: one of the primary reasons for the RemoveDIs project is not needlessly putting debug-info into blocks and making it part of the total-order-of-everything, but output consistency with dbg.value mode relies on that, so we have to give up on identical-outputs at some point.
---
Full diff: https://github.com/llvm/llvm-project/pull/77930.diff
5 Files Affected:
- (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+1)
- (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+4)
- (modified) llvm/lib/Transforms/InstCombine/InstCombineInternal.h (+7)
- (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+145-7)
- (added) llvm/test/DebugInfo/instcombine-sink-latest-assignment.ll (+74)
``````````diff
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 8230070343e0c1..9b9409821910fe 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -85,6 +85,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
public:
void deleteInstr();
+ const Instruction *getInstruction() const;
const BasicBlock *getParent() const;
BasicBlock *getParent();
void dump() const;
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 7b709a2de0335f..9fd17b1e979f1d 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -225,6 +225,10 @@ void DPValue::handleChangedLocation(Metadata *NewLocation) {
resetDebugValue(NewLocation);
}
+const Instruction *DPValue::getInstruction() const {
+ return Marker->MarkedInstr;
+}
+
const BasicBlock *DPValue::getParent() const {
return Marker->MarkedInstr->getParent();
}
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
index 21c61bd990184d..401b2bab76c453 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
+++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h
@@ -732,6 +732,13 @@ class LLVM_LIBRARY_VISIBILITY InstCombinerImpl final
Value *EvaluateInDifferentType(Value *V, Type *Ty, bool isSigned);
bool tryToSinkInstruction(Instruction *I, BasicBlock *DestBlock);
+ void tryToSinkInstructionDbgValues(
+ Instruction *I, BasicBlock::iterator InsertPos, BasicBlock *SrcBlock,
+ BasicBlock *DestBlock, SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers);
+ void tryToSinkInstructionDPValues(Instruction *I,
+ BasicBlock::iterator InsertPos,
+ BasicBlock *SrcBlock, BasicBlock *DestBlock,
+ SmallVectorImpl<DPValue *> &DPUsers);
bool removeInstructionsBeforeUnreachable(Instruction &I);
void addDeadEdge(BasicBlock *From, BasicBlock *To,
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 7f2018b3a19958..01e64ee40639d5 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -4217,12 +4217,29 @@ bool InstCombinerImpl::tryToSinkInstruction(Instruction *I,
// mark the location undef: we know it was supposed to receive a new location
// here, but that computation has been sunk.
SmallVector<DbgVariableIntrinsic *, 2> DbgUsers;
- findDbgUsers(DbgUsers, I);
+ SmallVector<DPValue *, 2> DPValues;
+ findDbgUsers(DbgUsers, I, &DPValues);
+ tryToSinkInstructionDbgValues(I, InsertPos, SrcBlock, DestBlock, DbgUsers);
+ tryToSinkInstructionDPValues(I, InsertPos, SrcBlock, DestBlock, DPValues);
+
+ // PS: there are numerous flaws with this behaviour, not least that right now
+ // assignments can be re-ordered past other assignments to the same variable
+ // if they use different Values. Creating more undef assignements can never be
+ // undone. And salvaging all users outside of this block can un-necessarily
+ // alter the lifetime of the live-value that the variable refers to.
+ // Some of these things can be resolved by tolerating debug use-before-defs in
+ // LLVM-IR, however it depends on the instruction-referencing CodeGen backend
+ // being used for more architectures.
+ return true;
+}
+
+void InstCombinerImpl::tryToSinkInstructionDbgValues(
+ Instruction *I, BasicBlock::iterator InsertPos, BasicBlock *SrcBlock,
+ BasicBlock *DestBlock, SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers) {
// For all debug values in the destination block, the sunk instruction
// will still be available, so they do not need to be dropped.
SmallVector<DbgVariableIntrinsic *, 2> DbgUsersToSalvage;
- SmallVector<DPValue *, 2> DPValuesToSalvage;
for (auto &DbgUser : DbgUsers)
if (DbgUser->getParent() != DestBlock)
DbgUsersToSalvage.push_back(DbgUser);
@@ -4266,10 +4283,7 @@ bool InstCombinerImpl::tryToSinkInstruction(Instruction *I,
// Perform salvaging without the clones, then sink the clones.
if (!DIIClones.empty()) {
- // RemoveDIs: pass in empty vector of DPValues until we get to instrumenting
- // this pass.
- SmallVector<DPValue *, 1> DummyDPValues;
- salvageDebugInfoForDbgValues(*I, DbgUsersToSalvage, DummyDPValues);
+ salvageDebugInfoForDbgValues(*I, DbgUsersToSalvage, {});
// The clones are in reverse order of original appearance, reverse again to
// maintain the original order.
for (auto &DIIClone : llvm::reverse(DIIClones)) {
@@ -4277,8 +4291,132 @@ bool InstCombinerImpl::tryToSinkInstruction(Instruction *I,
LLVM_DEBUG(dbgs() << "SINK: " << *DIIClone << '\n');
}
}
+}
- return true;
+void InstCombinerImpl::tryToSinkInstructionDPValues(
+ Instruction *I, BasicBlock::iterator InsertPos, BasicBlock *SrcBlock,
+ BasicBlock *DestBlock, SmallVectorImpl<DPValue *> &DPValues) {
+ // Implementation of tryToSinkInstructionDbgValues, but for the DPValue of
+ // variable assignments rather than dbg.values.
+
+ // Fetch all DPValues not already in the destination.
+ SmallVector<DPValue *, 2> DPValuesToSalvage;
+ for (auto &DPV : DPValues)
+ if (DPV->getParent() != DestBlock)
+ DPValuesToSalvage.push_back(DPV);
+
+ // Fetch a second collection, of DPValues in the source block that we're going
+ // to sink.
+ SmallVector<DPValue *> DPValuesToSink;
+ for (DPValue *DPV : DPValuesToSalvage)
+ if (DPV->getParent() == SrcBlock)
+ DPValuesToSink.push_back(DPV);
+
+ // Sort DPValues according to their position in the block. This is a partial
+ // order: DPValues attached to different instructions will be ordered by the
+ // instruction order, but DPValues attached to the same instruction won't
+ // have an order.
+ auto Order = [](DPValue *A, DPValue *B) -> bool {
+ return B->getInstruction()->comesBefore(A->getInstruction());
+ };
+ llvm::stable_sort(DPValuesToSink, Order);
+
+ // If there are two assignments to the same variable attached to the same
+ // instruction, the ordering between the two assignments is important. Scan
+ // for this (rare) case and establish which is the last assignment.
+ using InstVarPair = std::pair<const Instruction *, DebugVariable>;
+ SmallDenseMap<InstVarPair, DPValue *> FilterOutMap;
+ if (DPValuesToSink.size() > 1) {
+ SmallDenseMap<InstVarPair, unsigned> CountMap;
+ // Count how many assignments to each variable there is per instruction.
+ for (DPValue *DPV : DPValuesToSink) {
+ DebugVariable DbgUserVariable =
+ DebugVariable(DPV->getVariable(), DPV->getExpression(),
+ DPV->getDebugLoc()->getInlinedAt());
+ CountMap[std::make_pair(DPV->getInstruction(), DbgUserVariable)] += 1;
+ }
+
+ // If there are any instructions with two assignments, add them to the
+ // FilterOutMap to record that they need extra filtering.
+ SmallPtrSet<const Instruction *, 4> DupSet;
+ for (auto It : CountMap) {
+ if (It.second > 1) {
+ FilterOutMap[It.first] = nullptr;
+ DupSet.insert(It.first.first);
+ }
+ }
+
+ // For all instruction/variable pairs needing extra filtering, find the
+ // latest assignment.
+ for (const Instruction *Inst : DupSet) {
+ for (DPValue &DPV : llvm::reverse(Inst->getDbgValueRange())) {
+ DebugVariable DbgUserVariable =
+ DebugVariable(DPV.getVariable(), DPV.getExpression(),
+ DPV.getDebugLoc()->getInlinedAt());
+ auto FilterIt =
+ FilterOutMap.find(std::make_pair(Inst, DbgUserVariable));
+ if (FilterIt == FilterOutMap.end())
+ continue;
+ if (FilterIt->second != nullptr)
+ continue;
+ FilterIt->second = &DPV;
+ }
+ }
+ }
+
+ // Perform cloning of the DPValues that we plan on sinking, filter out any
+ // duplicate assignments identified above.
+ SmallVector<DPValue *, 2> DPVClones;
+ SmallSet<DebugVariable, 4> SunkVariables;
+ for (DPValue *DPV : DPValuesToSink) {
+ if (DPV->Type == DPValue::LocationType::Declare)
+ continue;
+
+ DebugVariable DbgUserVariable =
+ DebugVariable(DPV->getVariable(), DPV->getExpression(),
+ DPV->getDebugLoc()->getInlinedAt());
+
+ // For any variable where there were multiple assignments in the same place,
+ // ignore all but the last assignment.
+ if (!FilterOutMap.empty()) {
+ InstVarPair IVP = std::make_pair(DPV->getInstruction(), DbgUserVariable);
+ auto It = FilterOutMap.find(IVP);
+
+ // Filter out.
+ if (It != FilterOutMap.end() && It->second != DPV)
+ continue;
+ }
+
+ if (!SunkVariables.insert(DbgUserVariable).second)
+ continue;
+
+ // FIXME: dbg.assign equivalence. dbg.assigns here enter the SunkVariables
+ // map, but don't actually get sunk.
+
+ DPVClones.emplace_back(DPV->clone());
+ LLVM_DEBUG(dbgs() << "CLONE: " << *DPVClones.back() << '\n');
+ }
+
+ // Perform salvaging without the clones, then sink the clones.
+ if (DPVClones.empty())
+ return;
+
+ salvageDebugInfoForDbgValues(*I, {}, DPValuesToSalvage);
+
+ // The clones are in reverse order of original appearance. Assert that the
+ // head bit is set on the iterator as we _should_ have received it via
+ // getFirstInsertionPt. Inserting like this will reverse the clone order as
+ // we'll repeatedly insert at the head, such as:
+ // DPV-3 (third insertion goes here)
+ // DPV-2 (second insertion goes here)
+ // DPV-1 (first insertion goes here)
+ // Any-Prior-DPVs
+ // InsertPtInst
+ assert(InsertPos.getHeadBit());
+ for (DPValue *DPVClone : DPVClones) {
+ InsertPos->getParent()->insertDPValueBefore(DPVClone, InsertPos);
+ LLVM_DEBUG(dbgs() << "SINK: " << *DPVClone << '\n');
+ }
}
bool InstCombinerImpl::run() {
diff --git a/llvm/test/DebugInfo/instcombine-sink-latest-assignment.ll b/llvm/test/DebugInfo/instcombine-sink-latest-assignment.ll
new file mode 100644
index 00000000000000..cfa497750de18f
--- /dev/null
+++ b/llvm/test/DebugInfo/instcombine-sink-latest-assignment.ll
@@ -0,0 +1,74 @@
+; RUN: opt %s -o - -S --passes=instcombine | FileCheck %s
+; RUN: opt %s -o - -S --passes=instcombine --try-experimental-debuginfo-iterators | FileCheck %s
+;
+; CHECK-LABEL: for.body:
+; CHECK-NEXT: %sub.ptr.rhs.cast.i.i = ptrtoint ptr %call2.i.i to i64,
+; CHECK-NEXT: tail call void @llvm.dbg.value(metadata i64 %sub.ptr.rhs.cast.i.i, metadata !{{[0-9]*}}, metadata !DIExpression(DW_OP_LLVM_convert, 64, DW_ATE_unsigned, DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_constu, 1, DW_OP_minus, DW_OP_stack_value)
+;
+;; The code below is representative of a common situation: where we've had a
+;; loop be completely optimised out, leaving dbg.values representing the
+;; assignments for it, including a rotated assignment to the loop counter.
+;; Below, it's the two dbg.values in the entry block, assigning first the
+;; value of %conv.i, then the value of %conv.i minus one.
+;;
+;; When instcombine sinks %conv.i, it's critical that if it sinks a dbg.value
+;; with it, it sinks the most recent assignment. Otherwise it will re-order the
+;; assignments below, and a fall counter assignment will continue on from the
+;; end of the optimised-out loop.
+;;
+;; The check lines test that when the trunc sinks (along with the ptrtoint),
+;; we get the dbg.value with a DW_OP_minus in it.
+
+; ModuleID = 'tmp.ll'
+source_filename = "tmp.ll"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+define void @_ZN4llvm16MCObjectStreamer15EmitInstructionERKNS_6MCInstE(i1 %tobool.not) local_unnamed_addr {
+entry:
+ %call2.i.i = load volatile ptr, ptr null, align 8, !dbg !4
+ %sub.ptr.rhs.cast.i.i = ptrtoint ptr %call2.i.i to i64, !dbg !4
+ %conv.i = trunc i64 %sub.ptr.rhs.cast.i.i to i32, !dbg !4
+ tail call void @llvm.dbg.value(metadata i32 %conv.i, metadata !16, metadata !DIExpression()), !dbg !18
+ tail call void @llvm.dbg.value(metadata i32 %conv.i, metadata !16, metadata !DIExpression(DW_OP_constu, 1, DW_OP_minus, DW_OP_stack_value)), !dbg !18
+ br i1 %tobool.not, label %common.ret, label %for.body
+
+common.ret: ; preds = %for.body, %entry
+ ret void
+
+for.body: ; preds = %entry
+ %call2 = call ptr @_ZNK4llvm6MCInst10getOperandEj(i32 %conv.i)
+ br label %common.ret
+}
+
+declare i32 @_ZNK4llvm6MCInst14getNumOperandsEv()
+
+declare ptr @_ZNK4llvm6MCInst10getOperandEj(i32) local_unnamed_addr
+
+declare i64 @_ZNK4llvm25SmallVectorTemplateCommonINS_9MCOperandEvE4sizeEv()
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, globals: !2, imports: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/fast/fs/llvm34/lib/MC/MCObjectStreamer.cpp", directory: "/fast/fs/build34llvmstage", checksumkind: CSK_MD5, checksum: "43f3adff5ece50116e446307bd92824d")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !DILocation(line: 197, column: 26, scope: !5)
+!5 = distinct !DILexicalBlock(scope: !7, file: !6, line: 197, column: 3)
+!6 = !DIFile(filename: "llvm34/lib/MC/MCObjectStreamer.cpp", directory: "/fast/fs", checksumkind: CSK_MD5, checksum: "43f3adff5ece50116e446307bd92824d")
+!7 = distinct !DISubprogram(name: "EmitInstruction", linkageName: "_ZN4llvm16MCObjectStreamer15EmitInstructionERKNS_6MCInstE", scope: !8, file: !6, line: 195, type: !13, scopeLine: 195, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, declaration: !15, retainedNodes: !2)
+!8 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "MCObjectStreamer", scope: !10, file: !9, line: 33, size: 2432, flags: DIFlagTypePassByReference | DIFlagNonTrivial, elements: !2, vtableHolder: !11)
+!9 = !DIFile(filename: "llvm34/include/llvm/MC/MCObjectStreamer.h", directory: "/fast/fs", checksumkind: CSK_MD5, checksum: "364947c58883b0a72d98313c0775422d")
+!10 = !DINamespace(name: "llvm", scope: null)
+!11 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "MCStreamer", scope: !10, file: !12, line: 108, size: 2240, flags: DIFlagFwdDecl | DIFlagNonTrivial, identifier: "_ZTSN4llvm10MCStreamerE")
+!12 = !DIFile(filename: "llvm34/include/llvm/MC/MCStreamer.h", directory: "/fast/fs", checksumkind: CSK_MD5, checksum: "1fdd4f3a9a6a2340c2ba553eefe0e90b")
+!13 = distinct !DISubroutineType(types: !14)
+!14 = !{null}
+!15 = !DISubprogram(name: "EmitInstruction", linkageName: "_ZN4llvm16MCObjectStreamer15EmitInstructionERKNS_6MCInstE", scope: !8, file: !9, line: 88, type: !13, scopeLine: 88, containingType: !8, virtualIndex: 86, flags: DIFlagPublic | DIFlagPrototyped, spFlags: DISPFlagVirtual | DISPFlagOptimized)
+!16 = !DILocalVariable(name: "i", scope: !5, file: !6, line: 197, type: !17)
+!17 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!18 = !DILocation(line: 0, scope: !5)
``````````
</details>
https://github.com/llvm/llvm-project/pull/77930
More information about the llvm-commits
mailing list