[llvm] [DebugInfo][RemoveDIs] Avoid crash and output-difference in loop-rotate (PR #74093)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 1 07:47:50 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
Author: Jeremy Morse (jmorse)
<details>
<summary>Changes</summary>
Avoid editing a range of DPValues and then remapping them. This occurs when we try to de-duplicate dbg.values, but then re-use the same iterator range. We can instead remap them, and then erase any duplicates.
At the same time refactor the computation of seen-intrinsic hashes, and account for a peculiarity of loop-rotates existing behaviour: it will only deduplicate dbg.values that are immediately before the preheaders branch instruction, not just any dbg.value in the preheader.
---
Full diff: https://github.com/llvm/llvm-project/pull/74093.diff
2 Files Affected:
- (modified) llvm/lib/Transforms/Utils/LoopRotationUtils.cpp (+21-18)
- (added) llvm/test/Transforms/LoopRotate/delete-dbg-values.ll (+63)
``````````diff
diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
index cbef27d1ecfa68f..c6007aeac375faf 100644
--- a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
@@ -541,31 +541,30 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
// duplication.
using DbgIntrinsicHash =
std::pair<std::pair<hash_code, DILocalVariable *>, DIExpression *>;
- auto makeHash = [](DbgVariableIntrinsic *D) -> DbgIntrinsicHash {
+ auto makeHash = [](auto *D) -> DbgIntrinsicHash {
auto VarLocOps = D->location_ops();
return {{hash_combine_range(VarLocOps.begin(), VarLocOps.end()),
D->getVariable()},
D->getExpression()};
};
+
SmallDenseSet<DbgIntrinsicHash, 8> DbgIntrinsics;
for (Instruction &I : llvm::drop_begin(llvm::reverse(*OrigPreheader))) {
- if (auto *DII = dyn_cast<DbgVariableIntrinsic>(&I))
+ if (auto *DII = dyn_cast<DbgVariableIntrinsic>(&I)) {
DbgIntrinsics.insert(makeHash(DII));
- else
+ // Until RemoveDIs supports dbg.declares in DPValue format, we'll need
+ // to collect DPValues attached to any other debug intrinsics.
+ for (const DPValue &DPV : DII->getDbgValueRange())
+ DbgIntrinsics.insert(makeHash(&DPV));
+ } else {
break;
+ }
}
- // Duplicate implementation for DPValues, the non-instruction format of
- // debug-info records in RemoveDIs.
- auto makeHashDPV = [](const DPValue &D) -> DbgIntrinsicHash {
- auto VarLocOps = D.location_ops();
- return {{hash_combine_range(VarLocOps.begin(), VarLocOps.end()),
- D.getVariable()},
- D.getExpression()};
- };
- for (Instruction &I : llvm::drop_begin(llvm::reverse(*OrigPreheader)))
- for (const DPValue &DPV : I.getDbgValueRange())
- DbgIntrinsics.insert(makeHashDPV(DPV));
+ // Build DPValue hashes for DPValues attached to the terminator, which isn't
+ // considered in the loop above.
+ for (const DPValue &DPV : OrigPreheader->getTerminator()->getDbgValueRange())
+ DbgIntrinsics.insert(makeHash(&DPV));
// Remember the local noalias scope declarations in the header. After the
// rotation, they must be duplicated and the scope must be cloned. This
@@ -616,6 +615,10 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
LoopEntryBranch->cloneDebugInfoFrom(Inst, NextDbgInst);
RemapDPValueRange(M, DbgValueRange, ValueMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
+ // Erase anything we've seen before.
+ for (DPValue &DPV : make_early_inc_range(DbgValueRange))
+ if (DbgIntrinsics.count(makeHash(&DPV)))
+ DPV.eraseFromParent();
}
NextDbgInst = I->getDbgValueRange().begin();
@@ -633,13 +636,13 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat) {
auto Range = C->cloneDebugInfoFrom(Inst, NextDbgInst);
- // Erase anything we've seen before.
- for (DPValue &DPV : make_early_inc_range(Range))
- if (DbgIntrinsics.count(makeHashDPV(DPV)))
- DPV.eraseFromParent();
RemapDPValueRange(M, Range, ValueMap,
RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
NextDbgInst = std::nullopt;
+ // Erase anything we've seen before.
+ for (DPValue &DPV : make_early_inc_range(Range))
+ if (DbgIntrinsics.count(makeHash(&DPV)))
+ DPV.eraseFromParent();
}
// Eagerly remap the operands of the instruction.
diff --git a/llvm/test/Transforms/LoopRotate/delete-dbg-values.ll b/llvm/test/Transforms/LoopRotate/delete-dbg-values.ll
new file mode 100644
index 000000000000000..bce5ed02b43bf9a
--- /dev/null
+++ b/llvm/test/Transforms/LoopRotate/delete-dbg-values.ll
@@ -0,0 +1,63 @@
+; RUN: opt --passes=loop-rotate -o - -S %s | FileCheck %s --implicit-check-not=dbg.value
+; RUN: opt --passes=loop-rotate -o - -S %s --try-experimental-debuginfo-iterators | FileCheck %s --implicit-check-not=dbg.value
+;
+;; Test some fine-grained behaviour of loop-rotate's de-duplication of
+;; dbg.values. The intrinsic on the first branch should be seen and
+;; prevent the rotation of the dbg.value for "sink" into the entry block.
+;; However the other dbg.value, for "source", should not be seen, and we'll
+;; get a duplicate.
+;
+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"
+
+; CHECK: declare void @llvm.dbg.value(metadata,
+
+; CHECK-LABEL: define void @_ZNK4llvm5APInt4sextEj(ptr
+; CHECK-LABEL: entry:
+; CHECK: call void @llvm.dbg.value(metadata i32 0, metadata ![[SRC:[0-9]+]],
+; CHECK-NEXT: load
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0, metadata ![[SINK:[0-9]+]],
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0, metadata ![[SRC]],
+; CHECK-LABEL: for.body:
+; CHECK: call void @llvm.dbg.value(metadata i32 0, metadata ![[SINK]],
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 0, metadata ![[SRC]],
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+define void @_ZNK4llvm5APInt4sextEj(ptr %agg.result) !dbg !5 {
+entry:
+ tail call void @llvm.dbg.value(metadata i32 0, metadata !4, metadata !DIExpression()), !dbg !10
+ %.pre = load i32, ptr %agg.result, align 8
+ tail call void @llvm.dbg.value(metadata i32 0, metadata !11, metadata !DIExpression()), !dbg !10
+ br label %for.cond
+
+for.cond: ; preds = %for.body, %entry
+ %i.0 = phi i32 [ 0, %entry ], [ 1, %for.body ]
+ tail call void @llvm.dbg.value(metadata i32 0, metadata !11, metadata !DIExpression()), !dbg !10
+ tail call void @llvm.dbg.value(metadata i32 0, metadata !4, metadata !DIExpression()), !dbg !10
+ %cmp12.not = icmp eq i32 %i.0, %.pre, !dbg !10
+ br i1 %cmp12.not, label %for.end, label %for.body
+
+for.body: ; preds = %for.cond
+ store i64 0, ptr %agg.result, align 8
+ br label %for.cond
+
+for.end: ; preds = %for.cond
+ ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, globals: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "foo", directory: "bar")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !DILocalVariable(name: "source", scope: !5, file: !6, line: 170, type: !8)
+!5 = distinct !DISubprogram(name: "ConvertUTF16toUTF32", scope: !6, file: !6, line: 166, type: !7, scopeLine: 168, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!6 = !DIFile(filename: "fooo", directory: ".")
+!7 = !DISubroutineType(types: !2)
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !9, size: 64)
+!9 = !DIBasicType(name: "unsigned short", size: 16, encoding: DW_ATE_unsigned)
+!10 = !DILocation(line: 0, scope: !5)
+!11 = !DILocalVariable(name: "sink", scope: !5, file: !6, line: 170, type: !8)
``````````
</details>
https://github.com/llvm/llvm-project/pull/74093
More information about the llvm-commits
mailing list