[llvm] [DebugInfo][RemoveDIs] Avoid crash and output-difference in loop-rotate (PR #74093)
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 5 03:18:30 PST 2023
https://github.com/jmorse updated https://github.com/llvm/llvm-project/pull/74093
>From 2ed9d056a88a36cdbafa5de71802485b1eb64872 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Fri, 1 Dec 2023 00:58:46 +0000
Subject: [PATCH 1/2] [DebugInfo][RemoveDIs] Avoid crash and output-difference
in loop-rotate
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.
---
.../Transforms/Utils/LoopRotationUtils.cpp | 39 ++++++------
.../LoopRotate/delete-dbg-values.ll | 63 +++++++++++++++++++
2 files changed, 84 insertions(+), 18 deletions(-)
create mode 100644 llvm/test/Transforms/LoopRotate/delete-dbg-values.ll
diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
index cbef27d1ecfa6..c6007aeac375f 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 0000000000000..bce5ed02b43bf
--- /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)
>From f62d2a26451417572e16abf42640b38fabd26e2e Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Tue, 5 Dec 2023 11:17:11 +0000
Subject: [PATCH 2/2] clang-format
---
llvm/lib/Transforms/Utils/LoopRotationUtils.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
index c6007aeac375f..76280ed492b3d 100644
--- a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
@@ -563,7 +563,8 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
// Build DPValue hashes for DPValues attached to the terminator, which isn't
// considered in the loop above.
- for (const DPValue &DPV : OrigPreheader->getTerminator()->getDbgValueRange())
+ for (const DPValue &DPV :
+ OrigPreheader->getTerminator()->getDbgValueRange())
DbgIntrinsics.insert(makeHash(&DPV));
// Remember the local noalias scope declarations in the header. After the
More information about the llvm-commits
mailing list