[DebugInfo][RemoveDIs] Avoid crash and output-difference in loop-rotate

[jmorse]

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.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

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 --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)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[OCHyams]

LGTM