Page MenuHomePhabricator

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

Authored by yuanboli233 on Apr 13 2021, 2:04 AM.

Details

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.

Diff Detail

Event Timeline

yuanboli233 created this revision.Apr 13 2021, 2:04 AM
yuanboli233 requested review of this revision.Apr 13 2021, 2:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 2:04 AM
Orlando added inline comments.
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.

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

@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.

Updated with the simplified test case suggested by @Orlando.

@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 updated this revision to Diff 337233.EditedApr 13 2021, 12:38 PM

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.

yuanboli233 marked 3 inline comments as done.Apr 13 2021, 6:46 PM

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

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

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?

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

aprantl accepted this revision.May 12 2021, 6:29 PM
This revision is now accepted and ready to land.May 12 2021, 6:29 PM