<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><br class=""><blockquote type="cite" class="">On Jul 6, 2020, at 12:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:<br class=""><br class="">On Fri, Jun 26, 2020 at 5:18 PM Vedant Kumar via llvm-commits<br class=""><<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class=""><blockquote type="cite" class=""><br class=""><br class="">Author: Vedant Kumar<br class="">Date: 2020-06-26T17:18:15-07:00<br class="">New Revision: 9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34<br class=""><br class="">URL: <a href="https://github.com/llvm/llvm-project/commit/9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34" class="">https://github.com/llvm/llvm-project/commit/9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34</a><br class="">DIFF: <a href="https://github.com/llvm/llvm-project/commit/9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34.diff" class="">https://github.com/llvm/llvm-project/commit/9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34.diff</a><br class=""><br class="">LOG: [InstCombine] Drop debug loc in TryToSinkInstruction (reland)<br class=""><br class="">Summary:<br class="">The advice in HowToUpdateDebugInfo.rst is to "... preserve the debug<br class="">location of an instruction if the instruction either remains in its<br class="">basic block, or if its basic block is folded into a predecessor that<br class="">branches unconditionally".<br class=""><br class="">TryToSinkInstruction doesn't seem to satisfy the criteria as it's<br class="">sinking an instruction to some successor block. Preserving the debug loc<br class="">can make single-stepping appear to go backwards, or make a breakpoint<br class="">hit on that location happen "too late" (since single-stepping from that<br class="">breakpoint can cause the function to return unexpectedly).<br class=""><br class="">So, drop the debug location.<br class=""><br class="">This was reverted in ee3620643dfc because it removed source locations<br class="">from inlinable calls, breaking a verifier rule. I've added an exception<br class="">for calls because the alternative (setting a line 0 location) is not<br class="">better.<br class=""></blockquote><br class="">What do you mean by "is not better"?<br class=""><br class="">My understanding is that the other reason for not moving such<br class="">locations is profile accuracy. If the line is preserved when an<br class="">instruction (including a call) is sunk into a codepath that doesn't<br class="">necessarily pass through the original location, a sample based profile<br class="">could incorrectly conclude that a conditional was taken that would've<br class="">reached the original location of the instruction.<br class=""></blockquote><div class=""><br class=""></div><div class="">Sorry for the delay here.</div><div class=""><br class=""></div><div class="">In TryToSinkInstruction(InstToSink, DestBlock), it's required that InstToSink initially dominate DestBlock. Given a sample, the same conditions should appear to be true when not sinking vs. when sinking with the instruction location preserved.</div><div class=""><br class=""></div><div class="">That's why I think the main concern in TryToSinkInstruction is what happens to stepping behavior. The <a href="https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location" class="">preferred approach</a> would be to drop the DebugLoc, but that's not possible for an inlinable call instruction. The alternatives are to either set an artificial line 0 location or to preserve the location. I chose the latter because it's more precise: both approaches can make source-stepping seem jumpy, but preserving the location would at least show the user where they are.</div><div class=""><br class=""></div><div class="">vedant</div><br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class="">I tested the updated patch by completing a stage2 RelWithDebInfo<br class="">build.<br class=""><br class="">Reviewers: aprantl, davide<br class=""><br class="">Subscribers: hiraditya, llvm-commits<br class=""><br class="">Tags: #llvm<br class=""><br class="">Differential Revision: <a href="https://reviews.llvm.org/D82487" class="">https://reviews.llvm.org/D82487</a><br class=""><br class="">Added:<br class=""> llvm/test/Transforms/InstCombine/sink_to_unreachable_dbg.ll<br class=""><br class="">Modified:<br class=""> llvm/lib/Transforms/InstCombine/InstructionCombining.cpp<br class=""><br class="">Removed:<br class=""><br class=""><br class=""><br class="">################################################################################<br class="">diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp<br class="">index 1f97f0c1ac99..6a406314f63c 100644<br class="">--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp<br class="">+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp<br class="">@@ -3355,6 +3355,12 @@ static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {<br class=""> I->moveBefore(&*InsertPos);<br class=""> ++NumSunkInst;<br class=""><br class="">+ // Drop the debug loc of non-inlinable instructions. This prevents<br class="">+ // single-stepping from going backwards. See HowToUpdateDebugInfo.rst for<br class="">+ // the full rationale.<br class="">+ if (!isa<CallBase>(I))<br class="">+ I->setDebugLoc(DebugLoc());<br class="">+<br class=""> // Also sink all related debug uses from the source basic block. Otherwise we<br class=""> // get debug use before the def. Attempt to salvage debug uses first, to<br class=""> // maximise the range variables have location for. If we cannot salvage, then<br class=""><br class="">diff --git a/llvm/test/Transforms/InstCombine/sink_to_unreachable_dbg.ll b/llvm/test/Transforms/InstCombine/sink_to_unreachable_dbg.ll<br class="">new file mode 100644<br class="">index 000000000000..e642276224b8<br class="">--- /dev/null<br class="">+++ b/llvm/test/Transforms/InstCombine/sink_to_unreachable_dbg.ll<br class="">@@ -0,0 +1,46 @@<br class="">+; RUN: opt -debugify -debugify-level=locations -instcombine -S < %s | FileCheck %s<br class="">+<br class="">+; CHECK-LABEL: @test1(<br class="">+; CHECK: [[phi:%.*]] = phi i32<br class="">+; CHECK-NEXT: [[add:%.*]] = add i32 {{.*}}, 1{{$}}<br class="">+; CHECK-NEXT: add i32 [[phi]], [[add]], !dbg<br class="">+define i32 @test1(i32 %0, i1 %1) {<br class="">+ %3 = add i32 %0, 1<br class="">+ br i1 %1, label %4, label %5<br class="">+<br class="">+4: ; preds = %2<br class="">+ br label %6<br class="">+<br class="">+5: ; preds = %2<br class="">+ br label %6<br class="">+<br class="">+6: ; preds = %5, %4<br class="">+ %7 = phi i32 [ 0, %4 ], [ 1, %5 ]<br class="">+ %8 = add i32 %7, %3<br class="">+ ret i32 %8<br class="">+}<br class="">+<br class="">+; Function Attrs: nounwind readnone<br class="">+declare i32 @external(i32) #0<br class="">+<br class="">+; CHECK-LABEL: @test2(<br class="">+; CHECK: [[phi:%.*]] = phi i32<br class="">+; CHECK-NEXT: [[add:%.*]] = call i32 @external(i32 {{.*}}), !dbg<br class="">+; CHECK-NEXT: add i32 [[phi]], [[add]], !dbg<br class="">+define i32 @test2(i32 %0, i1 %1) {<br class="">+ %3 = call i32 @external(i32 %0)<br class="">+ br i1 %1, label %4, label %5<br class="">+<br class="">+4: ; preds = %2<br class="">+ br label %6<br class="">+<br class="">+5: ; preds = %2<br class="">+ br label %6<br class="">+<br class="">+6: ; preds = %5, %4<br class="">+ %7 = phi i32 [ 0, %4 ], [ 1, %5 ]<br class="">+ %8 = add i32 %7, %3<br class="">+ ret i32 %8<br class="">+}<br class="">+<br class="">+attributes #0 = { nounwind readnone }<br class=""><br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></blockquote></blockquote><br class=""></body></html>