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