[debug-info] SCCP should preserve the debug location for an one-to-one instruction replacement

[yuanboli233]

Summary

This revision focuses on fixing the bug proposed here: https://bugs.llvm.org/show_bug.cgi?id=48722

In the original ll file:

%tmp9 = trunc i64 %tmp8 to i32, !dbg !29
call void @llvm.dbg.value(metadata i32 %tmp9, metadata !18, metadata !DIExpression()), !dbg !29
%tmp10 = sext i32 %tmp9 to i64, !dbg !30
call void @llvm.dbg.value(metadata i64 %tmp10, metadata !19, metadata !DIExpression()), !dbg !30

is optimized to

%tmp9 = trunc i64 %tmp8 to i32, !dbg !29
call void @llvm.dbg.value(metadata i32 %tmp9, metadata !18, metadata !DIExpression()), !dbg !29
%0 = zext i32 %tmp9 to i64
call void @llvm.dbg.value(metadata i64 %0, metadata !19, metadata !DIExpression()), !dbg !30

There exists a one-to-one relationship between %tmp10 = sext i32 %tmp9 to i64, !dbg !30 and %0 = zext i32 %tmp9 to i64, however, the debug location is unnecessarily dropped.

This revision proposes to preserve the debug location !30 of the original instruction.

[Orlando]

llvm/test/Transforms/SCCP/sccp_preserve_debugloc.ll

11

It doesn't look like you need the llvm.dbg.value intrinsics in this test. Did you use opt --debugify to generate the debug info? I think you can use --debugify-level=locations to prevent debugify generating these.

[djtodoro]

@yuanboli233 Thanks for doing this. I was wondering how did you find these cases? By using debugify?

[yuanboli233]

@Orlando, thanks for pointing out the debugify usage for me. I indeed use debugify to generate the test cases. I will update test case soon.

@djtodoro, hope it is useful for the debug information updates! We are developing a static analysis tool based on the how-to-update-debug-info guide provided by the community. Some of the test cases are reduced from the test suite which can trigger the potential buggy code location. Some are generated using fuzzing and debugify.

[yuanboli233]

Updated with the simplified test case suggested by @Orlando.

[Orlando]

In D100371#2685353, @yuanboli233 wrote:

@Orlando, thanks for pointing out the debugify usage for me. I indeed use debugify to generate the test cases. I will update test case soon.

@djtodoro, hope it is useful for the debug information updates! We are developing a static analysis tool based on the how-to-update-debug-info guide provided by the community. Some of the test cases are reduced from the test suite which can trigger the potential buggy code location. Some are generated using fuzzing and debugify.

That sounds interesting!

llvm/test/Transforms/SCCP/sccp_preserve_debugloc.ll

3

Nit: Please can you capitalize 'check' and add a full stop to this sentence? I'm not sure if this is specified in the style guide but I tend to treat comments in tests the same as comments in source code.

4

Rather than checking for the presence of any !dbg I think you could be more specific and check that the correct source location is propagated here?

[yuanboli233]

Thanks for the suggestion, @Orlando.

One way to check whether the debug location is preserved is to keep one llvm.dbg.value call which has the same debug location with the replaced instruction. So I keep one of the llvm.dbg.value calls in the updated test case.

[djtodoro]

In D100371#2685353, @yuanboli233 wrote:

@Orlando, thanks for pointing out the debugify usage for me. I indeed use debugify to generate the test cases. I will update test case soon.

@djtodoro, hope it is useful for the debug information updates! We are developing a static analysis tool based on the how-to-update-debug-info guide provided by the community. Some of the test cases are reduced from the test suite which can trigger the potential buggy code location. Some are generated using fuzzing and debugify.

Cool -- are you planning to share it with community or (it might be a downstream project)?

[djtodoro]

I think one more place can be improved (I am using https://llvm.org/docs/HowToUpdateDebugInfo.html#test-original-debug-info-preservation-in-optimizations):

diff --git a/llvm/lib/Transforms/Scalar/SCCP.cpp b/llvm/lib/Transforms/Scalar/SCCP.cpp
index 86705b8622a3..3ac195a5c01a 100644
--- a/llvm/lib/Transforms/Scalar/SCCP.cpp
+++ b/llvm/lib/Transforms/Scalar/SCCP.cpp
@@ -1903,7 +1903,8 @@ static bool removeNonFeasibleEdges(const SCCPSolver &Solver, BasicBlock *BB,
       Updates.push_back({DominatorTree::Delete, BB, Succ});
     }

-    BranchInst::Create(OnlyFeasibleSuccessor, BB);
+    auto BranchInst = BranchInst::Create(OnlyFeasibleSuccessor, BB);
+    BranchInst->setDebugLoc(TI->getDebugLoc());
     TI->eraseFromParent();
     DTU.applyUpdatesPermissive(Updates);
   } else if (FeasibleSuccessors.size() > 1) {

[yuanboli233]

Yes, @djtodoro, it is in the starting phase, and we plan to improve it and make it an open-source project.

For the BranchInst case you mentioned, currently, I don't have a test case for it yet. Let me check whether I can generate a test case for it quickly to illustrate the problem. or do you think it is fine to modify it without an illustrating test case?

[djtodoro]

In D100371#2689076, @yuanboli233 wrote:

Yes, @djtodoro, it is in the starting phase, and we plan to improve it and make it an open-source project.

OK, thanks.

For the BranchInst case you mentioned, currently, I don't have a test case for it yet. Let me check whether I can generate a test case for it quickly to illustrate the problem. or do you think it is fine to modify it without an illustrating test case?

We need a test case for sure.
I've just compiled GDB project with -Xclang -fverify-debuginfo-preserve enabled, and found out that some zext and branches don't have !dbg after SCCP. (I haven't taken a closer look into the SCCP to see if the !dbg is valid to the new branch).

[Orlando]

Just going through some old reviews - it looks this bug still occurs but the patch is out of date. AFAICT the responsible code has been moved from SCCP.cpp to replaceSignedInst in SCCPSolver.cpp. @yuanboli233 are you able to fix this? I am happy to take over the patch if needed.

llvm/lib/Transforms/Scalar/SCCP.cpp

if (tryToReplaceWithConstant(Solver, &Inst)) {
       Value *ExtOp = Inst.getOperand(0);
       if (isa<Constant>(ExtOp) || InsertedValues.count(ExtOp))
         continue;
       const ValueLatticeElement &IV = Solver.getLatticeValueFor(ExtOp);
       if (!IV.isConstantRange(/*UndefAllowed=*/false))
         continue;
       if (IV.getConstantRange().isAllNonNegative()) {
         auto *ZExt = new ZExtInst(ExtOp, Inst.getType(), "", &Inst);
+        ZExt->setDebugLoc(Inst.getDebugLoc());
         InsertedValues.insert(ZExt);
         Inst.replaceAllUsesWith(ZExt);
         Solver.removeLatticeValueFor(&Inst);
         Inst.eraseFromParent();
         InstReplacedStat++;
         MadeChanges = true;
       }
     }

llvm/test/Transforms/SCCP/sccp_preserve_debugloc.ll

+; RUN: opt -sccp -S < %s | FileCheck %s
+; COM: Check whether the debug location is preserved.
+; CHECK: %{{[a-zA-Z0-9_]*}} = zext i32 %tmp9 to i64, !dbg ![[DBGLOC:[0-9]+]]
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 %{{[0-9]+}}, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg ![[DBGLOC]]
+source_filename = "abc.ll"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foo(i64* %arg) !dbg !6 {
+bb:
+  %tmp = zext i8 undef to i32, !dbg !20
+  %tmp1 = getelementptr inbounds i64, i64* %arg, i32 0, !dbg !21
+  %tmp2 = load i64, i64* %tmp1, align 8, !dbg !22
+  switch i32 %tmp, label %bb20 [
+    i32 1, label %bb3
+    i32 2, label %bb4
+    i32 4, label %bb19
+  ], !dbg !23
+
+bb3:                                              ; preds = %bb
+  unreachable, !dbg !24
+
+bb4:                                              ; preds = %bb
+  %tmp5 = add i64 %tmp2, 3, !dbg !25
+  %tmp6 = and i64 %tmp5, 3, !dbg !26
+  %tmp7 = sub i64 3, %tmp6, !dbg !27
+  %tmp8 = shl i64 %tmp7, 1, !dbg !28
+  %tmp9 = trunc i64 %tmp8 to i32, !dbg !29
+  %tmp10 = sext i32 %tmp9 to i64, !dbg !30
+  call void @llvm.dbg.value(metadata i64 %tmp10, metadata !19, metadata !DIExpression()), !dbg !30
+  unreachable, !dbg !31
+
+bb19:                                             ; preds = %bb
+  unreachable, !dbg !32
+
+bb20:                                             ; preds = %bb
+  ret void, !dbg !33
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "abc_simp.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 14}
+!4 = !{i32 9}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!19}
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!12 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+!19 = !DILocalVariable(name: "9", scope: !6, file: !1, line: 11, type: !12)
+!20 = !DILocation(line: 1, column: 1, scope: !6)
+!21 = !DILocation(line: 2, column: 1, scope: !6)
+!22 = !DILocation(line: 3, column: 1, scope: !6)
+!23 = !DILocation(line: 4, column: 1, scope: !6)
+!24 = !DILocation(line: 5, column: 1, scope: !6)
+!25 = !DILocation(line: 6, column: 1, scope: !6)
+!26 = !DILocation(line: 7, column: 1, scope: !6)
+!27 = !DILocation(line: 8, column: 1, scope: !6)
+!28 = !DILocation(line: 9, column: 1, scope: !6)
+!29 = !DILocation(line: 10, column: 1, scope: !6)
+!30 = !DILocation(line: 11, column: 1, scope: !6)
+!31 = !DILocation(line: 12, column: 1, scope: !6)
+!32 = !DILocation(line: 13, column: 1, scope: !6)
+!33 = !DILocation(line: 14, column: 1, scope: !6)