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