[DebugInfo][LoopLoadElim] Fix missing debug location updates #91839
Conversation
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-transforms Author: Shan Huang (Apochens) ChangesFix #91836 . For LoopLoadElim-L447 ( All the three updates are checked in the new test. Full diff: https://github.com/llvm/llvm-project/pull/91839.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp b/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
index f611ef6b2fa2d..60274f01f747f 100644
--- a/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
@@ -448,6 +448,10 @@ class LoadEliminationForLoop {
new LoadInst(Cand.Load->getType(), InitialPtr, "load_initial",
/* isVolatile */ false, Cand.Load->getAlign(),
PH->getTerminator()->getIterator());
+ // We don't give any debug location to Initial, because it is inserted
+ // into the loop's preheader. A debug location inside the loop will cause
+ // a misleading stepping when debugging. The test preserving-debugloc-store
+ // -forwarded.ll checks this.
PHINode *PHI = PHINode::Create(Initial->getType(), 2, "store_forwarded");
PHI->insertBefore(L->getHeader()->begin());
@@ -462,14 +466,20 @@ class LoadEliminationForLoop {
"The type sizes should match!");
Value *StoreValue = Cand.Store->getValueOperand();
- if (LoadType != StoreType)
+ if (LoadType != StoreType) {
StoreValue = CastInst::CreateBitOrPointerCast(StoreValue, LoadType,
"store_forward_cast",
Cand.Store->getIterator());
+ // Because it casts the old `load` value and is used by the new `phi`
+ // which replaces the old `load`, we give the `load`'s debug location
+ // to it.
+ cast<Instruction>(StoreValue)->setDebugLoc(Cand.Load->getDebugLoc());
+ }
PHI->addIncoming(StoreValue, L->getLoopLatch());
Cand.Load->replaceAllUsesWith(PHI);
+ PHI->setDebugLoc(Cand.Load->getDebugLoc());
}
/// Top-level driver for each loop: find store->load forwarding
diff --git a/llvm/test/Transforms/LoopLoadElim/update_debugloc_store_forwarded.ll b/llvm/test/Transforms/LoopLoadElim/update_debugloc_store_forwarded.ll
new file mode 100644
index 0000000000000..cf09e0571c0c6
--- /dev/null
+++ b/llvm/test/Transforms/LoopLoadElim/update_debugloc_store_forwarded.ll
@@ -0,0 +1,85 @@
+; RUN: opt -passes=loop-load-elim -S < %s | FileCheck %s
+
+; LoopLoadElimination's propagateStoredValueToLoadUsers() replaces the
+; `load` (`%a`) with an hoisted initial `load` and a `phi` that forwards
+; the stored value.
+; This test checks that the debug location is propagated to the new `phi`
+; from the original `load` it replaces in block `%for.body` and the debug
+; location drop of the hoisted `load` in block `%entry`.
+; Moreover, this test also checks the debug location update of the new
+; `bitcast` created when the `load` type is mismatched with the `store` type:
+; store i32 ...
+; %a = load float, ...
+; Because the `bitcast` casts the old `load` value, it has the same debug
+; location as the old `load` (ie., the same as the new `phi`).
+
+; If the store and the load use different types, but have the same
+; size then we should still be able to forward the value.
+;
+; for (unsigned i = 0; i < 100; i++) {
+; A[i+1] = B[i] + 2;
+; C[i] = ((float*)A)[i] * 2;
+; }
+
+define void @f(ptr noalias %A, ptr noalias %B, ptr noalias %C, i64 %N) !dbg !5 {
+; CHECK-LABEL: define void @f(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[LOAD_INITIAL:%.*]] = load float, ptr {{.*}}, align 4{{$}}
+; CHECK: for.body:
+; CHECK-NEXT: [[STORE_FORWARDED:%.*]] = phi float [ [[LOAD_INITIAL]], %entry ], [ [[STORE_FORWARD_CAST:%.*]], %for.body ], !dbg [[DBG9:![0-9]+]]
+; CHECK: [[STORE_FORWARD_CAST]] = bitcast i32 {{.*}} to float, !dbg [[DBG9]]
+; CHECK: [[DBG9]] = !DILocation(line: 11,
+;
+entry:
+ br label %for.body, !dbg !8
+
+for.body: ; preds = %for.body, %entry
+ %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ], !dbg !9
+ %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !10
+ %Aidx_next = getelementptr inbounds i32, ptr %A, i64 %indvars.iv.next, !dbg !11
+ %Bidx = getelementptr inbounds i32, ptr %B, i64 %indvars.iv, !dbg !12
+ %Cidx = getelementptr inbounds i32, ptr %C, i64 %indvars.iv, !dbg !13
+ %Aidx = getelementptr inbounds i32, ptr %A, i64 %indvars.iv, !dbg !14
+ %b = load i32, ptr %Bidx, align 4, !dbg !15
+ %a_p1 = add i32 %b, 2, !dbg !16
+ store i32 %a_p1, ptr %Aidx_next, align 4, !dbg !17
+ %a = load float, ptr %Aidx, align 4, !dbg !18
+ %c = fmul float %a, 2.000000e+00, !dbg !19
+ %c.int = fptosi float %c to i32, !dbg !20
+ store i32 %c.int, ptr %Cidx, align 4, !dbg !21
+ %exitcond = icmp eq i64 %indvars.iv.next, %N, !dbg !22
+ br i1 %exitcond, label %for.end, label %for.body, !dbg !23
+
+for.end: ; preds = %for.body
+ ret void, !dbg !24
+}
+
+!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: "type-mismatch.ll", directory: "/")
+!2 = !{i32 17}
+!3 = !{i32 0}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "f", linkageName: "f", 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)
+!16 = !DILocation(line: 9, column: 1, scope: !5)
+!17 = !DILocation(line: 10, column: 1, scope: !5)
+!18 = !DILocation(line: 11, column: 1, scope: !5)
+!19 = !DILocation(line: 12, column: 1, scope: !5)
+!20 = !DILocation(line: 13, column: 1, scope: !5)
+!21 = !DILocation(line: 14, column: 1, scope: !5)
+!22 = !DILocation(line: 15, column: 1, scope: !5)
+!23 = !DILocation(line: 16, column: 1, scope: !5)
+!24 = !DILocation(line: 17, column: 1, scope: !5)
|
llvm/test/Transforms/LoopLoadElim/update_debugloc_store_forwarded.ll
Outdated
Show resolved
Hide resolved
llvm/test/Transforms/LoopLoadElim/update_debugloc_store_forwarded.ll
Outdated
Show resolved
Hide resolved
|
|
||
| PHI->addIncoming(StoreValue, L->getLoopLatch()); | ||
|
|
||
| Cand.Load->replaceAllUsesWith(PHI); | ||
| PHI->setDebugLoc(Cand.Load->getDebugLoc()); |
There was a problem hiding this comment.
This is an interesting case. When promoting store/loads I think we usually give phis a debugloc from the store (or if it's from stores, plural, it's a merged location). At least that's what we do in mem2reg. I think in this case using the load's DebugLoc makes intuitive sense - we're just eliminating the load here after all, but I'd be interested to hear what others think about this case too (cc @SLTozer , @adrian-prantl , @dwblaikie, @jryans - there's a comment at the top of the function that explains the transformation) and what you make of this @Apochens?
There was a problem hiding this comment.
Using the load's location here does make intuitive sense to me as well. I'm less sure what we've done for such cases historically though, so I'm interested to hear what others think as well.
There was a problem hiding this comment.
I think in this case using the load's DebugLoc makes intuitive sense - we're just eliminating the load here after all
That's what I agree with.
When promoting store/loads I think we usually give phis a debugloc from the store (or if it's from stores, plural, it's a merged location).
I'd like to know why give the store'd debug location to an instruction that replaces a load instruction. Are there some conventions or something?
There was a problem hiding this comment.
Hmm, should we ping other guys again? 🤔
There was a problem hiding this comment.
Probably best to wait a few more days first, as people may be on holiday or have a long queue to work through.
There was a problem hiding this comment.
Looking at the git history, it doesn't look like there's a particular reason for using the store instead of the load - the main point was to ensure that the scope was correct. I think using the load makes more sense, so current state SGTM.
There was a problem hiding this comment.
Seems like enough of us have appeared and all concluded the same thing here, so I think we can leave this part as it is.
|
Once the remaining review comments are addressed, I imagine this can be approved. 🙂 |
There was a problem hiding this comment.
As @jryans mentions in the inline comments, it looks like we've reached a consensus that the eliminated load's debug location is indeed suitable for the phi. If someone comes back to this and strongly disagrees we can look at it again and make adjustments as necessary.
LGTM, thanks!
Fix #91836 .
For LoopLoadElim-L447 (
Initial), its debug location should be dropped, because it is inserted in the loop's preheater, which is out of the loop. SinceInitialhas an empty debug location when being created, I just add a comment to explicitly point out this case and do nothing with its debug location.All the three updates are checked in the new test.