[DebugInfo][NewGVN] Salvage debug values of trivially dead instructions

[Apochens]

fix #149301

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Shan Huang (Apochens)

Changes

fix #147634

---

Full diff: https://github.com/llvm/llvm-project/pull/149304.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+1)
• (added) llvm/test/Transforms/NewGVN/salvage-trivially-dead-inst.ll (+58)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 7eeaaa0d99602..17c4fd9c2aae9 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -3044,6 +3044,7 @@ std::pair<unsigned, unsigned> NewGVN::assignDFSNumbers(BasicBlock *B,
     if (isInstructionTriviallyDead(&I, TLI)) {
       InstrDFS[&I] = 0;
       LLVM_DEBUG(dbgs() << "Skipping trivially dead instruction " << I << "\n");
+      salvageDebugInfo(I);
       markInstructionForDeletion(&I);
       continue;
     }
diff --git a/llvm/test/Transforms/NewGVN/salvage-trivially-dead-inst.ll b/llvm/test/Transforms/NewGVN/salvage-trivially-dead-inst.ll
new file mode 100644
index 0000000000000..1845cf6f0852c
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/salvage-trivially-dead-inst.ll
@@ -0,0 +1,58 @@
+; RUN: opt -passes=newgvn -S %s | FileCheck %s
+
+; Check that assignDFSNumbers() in NewGVN salvages the debug values of the
+; trivially dead instructions that are marked for deletion.
+
+; CHECK: #dbg_value(i8 %tmp, [[META11:![0-9]+]], !DIExpression(DW_OP_constu, 8, DW_OP_eq, DW_OP_stack_value), [[META26:![0-9]+]])
+; CHECK: [[META11]] = !DILocalVariable(name: "2"
+; CHECK: [[META26]] = !DILocation(line: 3
+
+define void @test13() !dbg !5 {
+bb:
+  br label %bb1
+
+bb1:
+  %tmp = load i8, ptr null, align 1
+  %tmp2 = icmp eq i8 %tmp, 8, !dbg !26
+    #dbg_value(i1 %tmp2, !11, !DIExpression(), !26)
+  br label %bb3
+
+bb3:
+  %tmp4 = phi ptr [ null, %bb1 ], [ %tmp6, %bb3 ]
+  %tmp5 = phi i32 [ undef, %bb1 ], [ %tmp9, %bb3 ]
+  %tmp6 = getelementptr i8, ptr %tmp4, i64 1
+  %tmp7 = load i8, ptr %tmp4, align 1
+  %tmp8 = sext i8 %tmp7 to i32
+  %tmp9 = mul i32 %tmp5, %tmp8
+  %tmp10 = load i8, ptr %tmp6, align 1
+  %tmp11 = icmp eq i8 %tmp10, 0
+  br i1 %tmp11, label %bb12, label %bb3
+
+bb12:
+  %tmp13 = phi i32 [ %tmp9, %bb3 ]
+  %tmp14 = icmp eq i32 %tmp13, 0
+  br i1 %tmp14, label %bb1, label %bb15
+
+bb15:
+  call void (...) @bar()
+  br label %bb1
+}
+
+declare void @bar(...)
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!2, !3}
+!llvm.module.flags = !{!4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "/app/example.ll", directory: "/")
+!2 = !{i32 18}
+!3 = !{i32 12}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "test13", linkageName: "test13", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!11}
+!10 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !5, file: !1, line: 3, type: !10)
+!26 = !DILocation(line: 3, column: 1, scope: !5)
\ No newline at end of file

[github-actions]

✅ With the latest revision this PR passed the undef deprecator.

[OCHyams]

Are there are other calls to markInstructionForDeletion that should be accompanied by a salvage? What's the consequences of putting the salvage call in markInstructionForDeletion instead?

[OCHyams]

Do we need all the instructions below, or is the following sufficient to exercise the behaviour?

define void @test13() !dbg !5 {
entry:
  %tmp = load i8, ptr null, align 1
  %tmp2 = icmp eq i8 %tmp, 8, !dbg !26
    #dbg_value(i1 %tmp2, !11, !DIExpression(), !26)
  ret void
}

If you do need the extra instructions, I think you'll want to replace the undef with poison.

[Apochens]

Thanks. I think this concise test case is enough.

[Apochens]

Are there are other calls to markInstructionForDeletion that should be accompanied by a salvage?

For now, I found this call to markInstructionForDeletion and the prior issue (#147511) should be accompanied by a salvage. The other ones mark instructions like store or phi instructions. For these instructions, we don't need to salvage the corresponding debug values (salvageDebugInfo cannot handle them).

What's the consequences of putting the salvage call in markInstructionForDeletion instead?

As discussed in the prior issue (#147511), placing the salvage call into the following loop would introduce unnecessary computation overhead of salvaging.

llvm-project/llvm/lib/Transforms/Scalar/NewGVN.cpp

Line 3527 in 95cb833

for (Instruction *ToErase : InstructionsToErase) {

Similarly, putting the salvage call into markInstructionForDeletion would lead to extra salvage operations on instructions like store or phi instructions marked for deletion. So, IMO, it's better to put the salvage at a place where fewer or no extra salvage operations would be introduced.

[OCHyams]

LGTM, thanks