<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:20 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"><br>
<br>
> On Jul 13, 2020, at 2:15 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> <br>
> <br>
> <br>
> On Mon, Jul 13, 2020 at 2:12 PM <<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>> wrote:<br>
> <br>
> <br>
>> 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>
>>> <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" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34</a><br>
>>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/9649c2095f07a392bc2b2a93b5bd6c4c9bf5ba34.diff" rel="noreferrer" 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>
>> <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>
> <br>
> Sorry for the delay here.<br>
> <br>
> 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.<br>
> <br>
> Ah - right, understood. Thanks for explaining!<br>
> <br>
> That's why I think the main concern in TryToSinkInstruction is what happens to stepping behavior. The preferred approach 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>
> <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>
<br>
Oh, that's a great point. The same argument really does apply for preserving the location for non-call instructions. This might warrant an update for HowToUpdateDebugInfo.rst.<br>
<br>
I'll go ahead and revert this for now and send a patch amending HowToUpdateDebugInfo.rst. I expect that we'll want to keep this reverted after the discussion, but if not, we can always re-apply. Does that sound all right?<br></blockquote><div><br></div><div>Works for me, yeah - thanks!<br><br>- Dave</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">
<br>
vedant<br>
<br>
> <br>
> <br>
> vedant<br>
> <br>
>> <br>
>>> 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" rel="noreferrer" 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" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div></div>