[DebugInfo][NewGVN] Fix debug value loss

[Apochens]

Fix #147511

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Shan Huang (Apochens)

Changes

Fix #147511

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+1)
• (added) llvm/test/Transforms/NewGVN/salvage-eliminate-instruction.ll (+43)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 7eeaaa0d99602..f101d3c126491 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -4076,6 +4076,7 @@ bool NewGVN::eliminateInstructions(Function &F) {
                 if (!match(DefI, m_Intrinsic<Intrinsic::ssa_copy>()))
                   patchReplacementInstruction(DefI, DominatingLeader);
 
+                salvageDebugInfo(*DefI);
                 markInstructionForDeletion(DefI);
               }
             }
diff --git a/llvm/test/Transforms/NewGVN/salvage-eliminate-instruction.ll b/llvm/test/Transforms/NewGVN/salvage-eliminate-instruction.ll
new file mode 100644
index 0000000000000..fe26cefb2803e
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/salvage-eliminate-instruction.ll
@@ -0,0 +1,43 @@
+; RUN: opt -S -passes=newgvn %s | FileCheck %s
+
+; Check that eliminateInstruction() salvages the debug value of `Def` (`DefI`)
+; which is marked for deletion.
+
+
+define void @binop(i32 %x, i32 %y) !dbg !5 {
+; CHECK: #dbg_value(!DIArgList(i32 %y, i32 %x), [[META11:![0-9]+]], !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value), [[META13:![0-9]+]])
+;
+  %add1 = add i32 %x, %y, !dbg !12
+    #dbg_value(i32 %add1, !9, !DIExpression(), !12)
+  %add2 = add i32 %y, %x, !dbg !13
+    #dbg_value(i32 %add2, !11, !DIExpression(), !13)
+  call void @use(i32 %add1, i32 %add2), !dbg !14
+  ret void, !dbg !15
+}
+
+declare void @use(i32, i32)
+
+!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 4}
+!3 = !{i32 2}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "binop", linkageName: "binop", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9, !11}
+!9 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !5, file: !1, line: 2, type: !10)
+!12 = !DILocation(line: 1, column: 1, scope: !5)
+!13 = !DILocation(line: 2, column: 1, scope: !5)
+!14 = !DILocation(line: 3, column: 1, scope: !5)
+!15 = !DILocation(line: 4, column: 1, scope: !5)
+;.
+; CHECK: [[META11]] = !DILocalVariable(name: "2",
+; CHECK: [[META13]] = !DILocation(line: 2,
+;.

[dzhidzhoev]

Should this be moved inside the loop

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

Line 3527 in 95cb833

for (Instruction *ToErase : InstructionsToErase) {

to salvage more instructions?

[Apochens]

That would be a solution, but that would introduce some unnecessary computation overhead of salvaging. For example, salvaging the store instructions in InstructionsToErase is unnecessary:

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

Lines 4194 to 4196 in 6c8c836

	LLVM_DEBUG(dbgs() << "Marking dead store " << *Member
	<< " that is dominated by " << *Leader << "\n");
	markInstructionForDeletion(Member);

So, I didn't place this salvage into the loop but place it just before where the salvageable instructions are marked for deletion. But if this unnecessary compulation is acceptable, I can move this salvage into the loop.

[dzhidzhoev]

Thank you. I'm fine with the current version if extra salvaging calls are redundant.

[OCHyams]

Could we replace the debug-use of %add2 with the %add1 instead of salvaging it?

[OCHyams]

The comment has a chance of going stale, referring to the LLVM source. I think function name is ok but instead of LLVM source variables, it's probably better to describe what's happening in terms of the IR in the test.

[Apochens]

Thanks, I have refined it in the latest commit.

[Apochens]

Could we replace the debug-use of %add2 with the %add1 instead of salvaging it?

@OCHyams I think we can do this replacement. In NewGVN, the instruction pointed by DefI (or Def) and that pointed by DominatingLeader are two equivalent instructions in the same congruence class, and the latter will replace the former.

[github-actions]

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

[OCHyams]

LGTM. Please make the clang-format bot happy

[OCHyams]

We're in the process of removing debug intrinsics completely, so it's fine to ignore DbgUsers. Also fine to leave this here for now, it will get cleaned up when findDbgUsers is updated. Just a note for future patches.