[DebugInfo][JumpThreading] Fix missing debug location updates #91581
Conversation
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-transforms Author: Shan Huang (Apochens) ChangesFix #91574.
@SLTozer Could you please help in assigning reviewers for this PR. Thanks! (Now I may not continously work on this project, so I think requesting the contributor permission could be inappropriate. I apologize for directly pinging you again and again.) It's difficult to construct a suitable test case for JumpThreading-L1282. It seems that the transformation happens rarely. Probably experienced reviewers can give some suggestions. Full diff: https://github.com/llvm/llvm-project/pull/91581.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 08d82fa66da30..f7778166bc4fb 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -1278,9 +1278,12 @@ bool JumpThreadingPass::simplifyPartiallyRedundantLoad(LoadInst *LoadI) {
// only happen in dead loops.
if (AvailableVal == LoadI)
AvailableVal = PoisonValue::get(LoadI->getType());
- if (AvailableVal->getType() != LoadI->getType())
+ if (AvailableVal->getType() != LoadI->getType()) {
AvailableVal = CastInst::CreateBitOrPointerCast(
AvailableVal, LoadI->getType(), "", LoadI->getIterator());
+ if (Instruction * CI = dyn_cast<Instruction>(AvailableVal))
+ CI->setDebugLoc(LoadI->getDebugLoc());
+ }
LoadI->replaceAllUsesWith(AvailableVal);
LoadI->eraseFromParent();
return true;
@@ -2983,6 +2986,7 @@ bool JumpThreadingPass::tryToUnfoldSelectInCurrBB(BasicBlock *BB) {
PHINode *NewPN = PHINode::Create(SI->getType(), 2, "", SI->getIterator());
NewPN->addIncoming(SI->getTrueValue(), Term->getParent());
NewPN->addIncoming(SI->getFalseValue(), BB);
+ NewBB->setDebugLoc(SI->getDebugLoc());
SI->replaceAllUsesWith(NewPN);
SI->eraseFromParent();
// NewBB and SplitBB are newly created blocks which require insertion.
@@ -3120,6 +3124,7 @@ bool JumpThreadingPass::threadGuard(BasicBlock *BB, IntrinsicInst *Guard,
PHINode *NewPN = PHINode::Create(Inst->getType(), 2);
NewPN->addIncoming(UnguardedMapping[Inst], UnguardedBlock);
NewPN->addIncoming(GuardedMapping[Inst], GuardedBlock);
+ NewPN->setDebugLoc(Inst->getDebugLoc());
NewPN->insertBefore(InsertionPoint);
Inst->replaceAllUsesWith(NewPN);
}
diff --git a/llvm/test/Transforms/JumpThreading/guard-split-debuginfo.ll b/llvm/test/Transforms/JumpThreading/guard-split-debuginfo.ll
index 05ff74939449c..203a8a9ae2395 100644
--- a/llvm/test/Transforms/JumpThreading/guard-split-debuginfo.ll
+++ b/llvm/test/Transforms/JumpThreading/guard-split-debuginfo.ll
@@ -7,6 +7,9 @@
; parent blocks. And that ino jump-threading, the old dbg.value gets
; deleted.
+; Test debug location preserving in function threadGuard, which replaces
+; instructions with PHINodes at the end of the function. [[TMP1]]
+
declare void @llvm.experimental.guard(i1, ...)
declare i32 @f1()
@@ -33,8 +36,9 @@ define i32 @branch_implies_guard(i32 %a) !dbg !7 {
; CHECK-NEXT: br label [[MERGE]]
; CHECK: Merge:
; CHECK-NEXT: [[RETPHI:%.*]] = phi i32 [ [[V1]], [[T1_SPLIT]] ], [ [[V2]], [[F1_SPLIT]] ]
-; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[RETVAL3]], [[T1_SPLIT]] ], [ [[RETVAL1]], [[F1_SPLIT]] ]
+; CHECK-NEXT: [[TMP1:%.*]] = phi i32 [ [[RETVAL3]], [[T1_SPLIT]] ], [ [[RETVAL1]], [[F1_SPLIT]] ], !dbg [[DBG12]]
; CHECK-NEXT: ret i32 [[TMP1]], !dbg [[DBG12]]
+; CHECK: [[DBG12]] = !DILocation(line: 13,
;
%cond = icmp slt i32 %a, 10
br i1 %cond, label %T1, label %F1, !dbg !26
diff --git a/llvm/test/Transforms/JumpThreading/preserving-debugloc-fold-select.ll b/llvm/test/Transforms/JumpThreading/preserving-debugloc-fold-select.ll
new file mode 100644
index 0000000000000..36137c23b42a5
--- /dev/null
+++ b/llvm/test/Transforms/JumpThreading/preserving-debugloc-fold-select.ll
@@ -0,0 +1,98 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -S -passes=jump-threading | FileCheck %s
+
+; Test the debug location update of the newly created PHINode
+; which replaces the select instruction in .exit block.
+
+define i32 @unfold3(i32 %u, i32 %v, i32 %w, i32 %x, i32 %y, i32 %z, i32 %j) !dbg !5 {
+; CHECK-LABEL: define i32 @unfold3(
+; CHECK-SAME: i32 [[U:%.*]], i32 [[V:%.*]], i32 [[W:%.*]], i32 [[X:%.*]], i32 [[Y:%.*]], i32 [[Z:%.*]], i32 [[J:%.*]]) !dbg [[DBG5:![0-9]+]] {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[ADD3:%.*]] = add nsw i32 [[J]], 2, !dbg [[DBG19:![0-9]+]]
+; CHECK-NEXT: [[CMP_I:%.*]] = icmp slt i32 [[U]], [[V]], !dbg [[DBG20:![0-9]+]]
+; CHECK-NEXT: br i1 [[CMP_I]], label [[DOTEXIT_THREAD4:%.*]], label [[COND_FALSE_I:%.*]], !dbg [[DBG21:![0-9]+]]
+; CHECK: cond.false.i:
+; CHECK-NEXT: [[CMP4_I:%.*]] = icmp sgt i32 [[U]], [[V]], !dbg [[DBG22:![0-9]+]]
+; CHECK-NEXT: br i1 [[CMP4_I]], label [[DOTEXIT_THREAD:%.*]], label [[COND_FALSE_6_I:%.*]], !dbg [[DBG23:![0-9]+]]
+; CHECK: cond.false.6.i:
+; CHECK-NEXT: [[CMP8_I:%.*]] = icmp slt i32 [[W]], [[X]], !dbg [[DBG24:![0-9]+]]
+; CHECK-NEXT: br i1 [[CMP8_I]], label [[DOTEXIT_THREAD4]], label [[COND_FALSE_10_I:%.*]], !dbg [[DBG25:![0-9]+]]
+; CHECK: cond.false.10.i:
+; CHECK-NEXT: [[CMP13_I:%.*]] = icmp sgt i32 [[W]], [[X]], !dbg [[DBG26:![0-9]+]]
+; CHECK-NEXT: br i1 [[CMP13_I]], label [[DOTEXIT_THREAD]], label [[DOTEXIT:%.*]], !dbg [[DBG27:![0-9]+]]
+; CHECK: .exit:
+; CHECK-NEXT: [[PHITMP:%.*]] = icmp sge i32 [[Y]], [[Z]], !dbg [[DBG28:![0-9]+]]
+; CHECK-NEXT: [[COND_FR:%.*]] = freeze i1 [[PHITMP]]
+; CHECK-NEXT: br i1 [[COND_FR]], label [[DOTEXIT_THREAD]], label [[DOTEXIT_THREAD4]], !dbg [[DBG29:![0-9]+]]
+; CHECK: .exit.thread:
+; CHECK-NEXT: br label [[DOTEXIT_THREAD4]], !dbg [[DBG29]]
+; CHECK: .exit.thread4:
+; CHECK-NEXT: [[TMP0:%.*]] = phi i32 [ [[J]], [[DOTEXIT_THREAD]] ], [ [[ADD3]], [[DOTEXIT]] ], [ [[ADD3]], [[ENTRY:%.*]] ], [ [[ADD3]], [[COND_FALSE_6_I]] ], !dbg [[DBG29]]
+; CHECK-NEXT: ret i32 [[TMP0]], !dbg [[DBG30:![0-9]+]]
+;
+; CHECK: [[DBG29]] = !DILocation(line: 13,
+;
+entry:
+ %add3 = add nsw i32 %j, 2, !dbg !19
+ %cmp.i = icmp slt i32 %u, %v, !dbg !20
+ br i1 %cmp.i, label %.exit, label %cond.false.i, !dbg !21
+
+cond.false.i: ; preds = %entry
+ %cmp4.i = icmp sgt i32 %u, %v, !dbg !22
+ br i1 %cmp4.i, label %.exit, label %cond.false.6.i, !dbg !23
+
+cond.false.6.i: ; preds = %cond.false.i
+ %cmp8.i = icmp slt i32 %w, %x, !dbg !24
+ br i1 %cmp8.i, label %.exit, label %cond.false.10.i, !dbg !25
+
+cond.false.10.i: ; preds = %cond.false.6.i
+ %cmp13.i = icmp sgt i32 %w, %x, !dbg !26
+ br i1 %cmp13.i, label %.exit, label %cond.false.15.i, !dbg !27
+
+cond.false.15.i: ; preds = %cond.false.10.i
+ %phitmp = icmp sge i32 %y, %z, !dbg !28
+ br label %.exit, !dbg !29
+
+.exit: ; preds = %cond.false.15.i, %cond.false.10.i, %cond.false.6.i, %cond.false.i, %entry
+ %cond23.i = phi i1 [ false, %entry ], [ true, %cond.false.i ], [ false, %cond.false.6.i ], [ %phitmp, %cond.false.15.i ], [ true, %cond.false.10.i ], !dbg !30
+ %j.add3 = select i1 %cond23.i, i32 %j, i32 %add3, !dbg !31
+ ret i32 %j.add3, !dbg !32
+}
+
+!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: "preserving-debugloc-trytofoldselect.ll", directory: "/")
+!2 = !{i32 14}
+!3 = !{i32 8}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "unfold3", linkageName: "unfold3", 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, !13, !14, !15, !16, !17, !18}
+!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: !12)
+!12 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
+!13 = !DILocalVariable(name: "3", scope: !5, file: !1, line: 4, type: !12)
+!14 = !DILocalVariable(name: "4", scope: !5, file: !1, line: 6, type: !12)
+!15 = !DILocalVariable(name: "5", scope: !5, file: !1, line: 8, type: !12)
+!16 = !DILocalVariable(name: "6", scope: !5, file: !1, line: 10, type: !12)
+!17 = !DILocalVariable(name: "7", scope: !5, file: !1, line: 12, type: !12)
+!18 = !DILocalVariable(name: "8", scope: !5, file: !1, line: 13, type: !10)
+!19 = !DILocation(line: 1, column: 1, scope: !5)
+!20 = !DILocation(line: 2, column: 1, scope: !5)
+!21 = !DILocation(line: 3, column: 1, scope: !5)
+!22 = !DILocation(line: 4, column: 1, scope: !5)
+!23 = !DILocation(line: 5, column: 1, scope: !5)
+!24 = !DILocation(line: 6, column: 1, scope: !5)
+!25 = !DILocation(line: 7, column: 1, scope: !5)
+!26 = !DILocation(line: 8, column: 1, scope: !5)
+!27 = !DILocation(line: 9, column: 1, scope: !5)
+!28 = !DILocation(line: 10, column: 1, scope: !5)
+!29 = !DILocation(line: 11, column: 1, scope: !5)
+!30 = !DILocation(line: 12, column: 1, scope: !5)
+!31 = !DILocation(line: 13, column: 1, scope: !5)
+!32 = !DILocation(line: 14, column: 1, scope: !5)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| if (Instruction *CI = dyn_cast<Instruction>(AvailableVal)) | ||
| CI->setDebugLoc(LoadI->getDebugLoc()); |
There was a problem hiding this comment.
| if (Instruction *CI = dyn_cast<Instruction>(AvailableVal)) | |
| CI->setDebugLoc(LoadI->getDebugLoc()); | |
| cast<Instruction>(AvailableVal)->setDebugLoc(LoadI->getDebugLoc()); |
We don't need the dyn_cast check because CastInst::CreateBitOrPointerCast returns an Instruction *.
There was a problem hiding this comment.
re: generating a test for this part, if you stick an assert(false) in here and run the build target check-llvm do we get any test failures?
There was a problem hiding this comment.
Yes. For each case like this, I would first search for existing tests by sticking an assert(false). If any test is found, I modify that test to obtain a test for the patched site. For this site, I have not found any existing test, which can trigger it.
There was a problem hiding this comment.
That's unfortunate that we have a hole in test coverage here but certainly not unprecedented. Another option could be to use the clang built with that assert to build other codebases, e.g., self-host (build clang again), the llvm-test-suite, or any other codebase you might have access to.
Alternatively, you could try to widen the search, e.g., moving the assert further into common code. If it's inside an if at the moment, move it outside the if (or further). When that gives you a test you may be able to modify the IR from that test by hand in order to hit the desired code path.
If you have a go at this and it's not fruitful let me know and I'll have a go at it too.
There was a problem hiding this comment.
Ok, I will try other codebases. (Actually, I have tried to remove the assertion out of the then branch and found a test file (test8). But I cannot modify it to obtain the desired one.)
There was a problem hiding this comment.
@OCHyams I have tried check-llvm, check-all and check-llvm-unit, resulting no test triggering the assertion (even if I move the assertion out of the then branch).
There was a problem hiding this comment.
The llvm-test-suite that @OCHyams is referencing isn't the test suite within LLVM (the confusion is understandable of course!), but the LLVM test suite repo, which contains a variety of real program selected to test LLVM; it contains a decent spread of programs (so likely to stimulate more code in LLVM) and is faster to build than a stage 2 clang build; there are instructions on how to use it here.
There was a problem hiding this comment.
Thanks for trying that out. I managed to stimulate this codepath by modifying the test you linked above - I changed %a's type to float (then ran debugify with the --debugify-level=locations option). I haven't added the RUN/CHECK lines but this should do it:
target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128"
target triple = "i386-apple-darwin7"
declare void @f1(...)
define void @test8(ptr %0, ptr %1, ptr %2) !dbg !5 {
%a = load float, ptr %0, align 4, !dbg !8
%b = load i32, ptr %0, align 4, !dbg !9
store float %a, ptr %1, align 4, !dbg !10
%c = icmp eq i32 %b, 8, !dbg !11
br i1 %c, label %ret1, label %ret2, !dbg !12
ret1: ; preds = %3
ret void, !dbg !13
ret2: ; preds = %3
%xxx = tail call i32 (...) @f1() #0, !dbg !14
ret void, !dbg !15
}
!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: "test.ll", directory: "/")
!2 = !{i32 8}
!3 = !{i32 0}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = distinct !DISubprogram(name: "test8", linkageName: "test8", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
!6 = !DISubroutineType(types: !7)
!7 = !{}
!8 = !DILocation(line: 1, column: 1, scope: !5)
!9 = !DILocation(line: 2, column: 1, scope: !5)
!10 = !DILocation(line: 3, column: 1, scope: !5)
!11 = !DILocation(line: 4, column: 1, scope: !5)
!12 = !DILocation(line: 5, column: 1, scope: !5)
!13 = !DILocation(line: 6, column: 1, scope: !5)
!14 = !DILocation(line: 7, column: 1, scope: !5)
!15 = !DILocation(line: 8, column: 1, scope: !5)
There was a problem hiding this comment.
The llvm-test-suite that @OCHyams is referencing isn't the test suite within LLVM
Ah, I had intended to link to it and forgot, thanks for clarifying that.
There was a problem hiding this comment.
The test works! I have already add checks into it and included it into this PR! Thanks!
| @@ -0,0 +1,98 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 | |||
There was a problem hiding this comment.
Is this true - it looks like there are additional checks you've added by hand (at least the DBG29 check)?
If someone came along and regenerated the checks using update_test_checks.py I'm worried we'd lose the ones you added (I'm not super familiar with update_test_checks.py - maybe you could check this out?).
As an aside, I'm not sure we need to check each instruction here either, just enough to be sure the transformation has happened - possibly it would be enough to just check the .exit.thread4: block contents?
There was a problem hiding this comment.
The checks in this test are generated by using update_test_checkes.py, and then I remove some redundant debug metadata checks and keep the DBG29. So, if someone regenerate the checks, the crucial checks would still be there.
For those instruction checks, I think we can only keep those of .exit.thread4.
There was a problem hiding this comment.
You should not modify or remove checks that were automatically generated. That's the point of automatic generation.
The automated scripts generate far more checks than are actually necessary, but removing some doesn't actually help anything.
There was a problem hiding this comment.
You should not modify or remove checks that were automatically generated. That's the point of automatic generation.
So, should I keep all the automatically generated checks? How about those redundant checks that are irrelevant to the fix in this case?
There was a problem hiding this comment.
I think removing the line ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 here should be enough for this one (discussion continues up in the other test though).
Otherwise the latest change to this one looks good, thanks.
There was a problem hiding this comment.
Right. If the test has a comment saying the tests were auto-generated, that comment needs to tell the truth (no manual modifications of the checks). It's likely the generated CHECKs will do more than you would in a manual test, but that's okay. People using the scripts expect that.
There was a problem hiding this comment.
Oh, I understand now! Thanks for pointing out this!
| @@ -7,6 +7,9 @@ | |||
| ; parent blocks. And that ino jump-threading, the old dbg.value gets | |||
| ; deleted. | |||
|
|
|||
| ; Test debug location preserving in function threadGuard, which replaces | |||
| ; instructions with PHINodes at the end of the function. [[TMP1]] | |||
There was a problem hiding this comment.
nit: "at the end of the function" are you referring to the code threadGuard? This feels like a comment that could go stale quite easily. I also find the first part a little hard to parse - perhaps the comment could be replaced with something along the lines of "Test that jump threading's threadGuard propagates the debug location to the phi from the instruction it replaces (%retVal)."?
also nit: [[TMP1]]?
My comment in the next test about utils/update_test_checks.py applies to this one too.
| ; CHECK-NEXT: ret i32 [[TMP1]], !dbg [[DBG12]] | ||
| ; CHECK: [[DBG12]] = !DILocation(line: 13, |
There was a problem hiding this comment.
The same concern mentioned below about auto-generated checks applies to this test. The problem is that this test is marked as auto generated so someone might re-generate the test in the future. In this case this would result in a loss of test coverage because the regenerated test would not include your CHECK: [[DBG12]] = ... directive.
IMO there are a couple of options to choose from:
- Use this file but check metadata too by using the update_test_checks.py argument
--check-globals - Duplicate this test file, then in the new file remove the "auto-generated" line at the top and the CHECK directives we don't need, and add your new CHECK(s).
- Use this file but remove the "auto-generated" line at the top.
Those options are more or less in my order of preference, if option 1 doesn't add too much noise to the test - could you try that out?
There was a problem hiding this comment.
I tried option 1, and checks for all the metadata are automatically generated, adding 17 new checks/lines. Would that be alright?
There was a problem hiding this comment.
Thanks, yeah I think that's okay for this one then.
| @@ -0,0 +1,98 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 | |||
There was a problem hiding this comment.
I think removing the line ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 here should be enough for this one (discussion continues up in the other test though).
Otherwise the latest change to this one looks good, thanks.
Fix #91574.
preserving-debugloc-fold-select.ll.guard-split-debuginfo.ll.@SLTozer Could you please help in assigning reviewers for this PR. Thanks! (Now I may not continously work on this project, so I think requesting the contributor permission could be inappropriate. I apologize for directly pinging you again and again.)
It's difficult to construct a suitable test case for JumpThreading-L1282. It seems that the transformation happens rarely. Probably experienced reviewers can give some suggestions.