[llvm] 9649c20 - [InstCombine] Drop debug loc in TryToSinkInstruction (reland)

Mon Jul 13 14:56:42 PDT 2020

On Mon, Jul 13, 2020 at 2:20 PM <vsk at apple.com> wrote:

>
>
> > On Jul 13, 2020, at 2:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Jul 13, 2020 at 2:12 PM <vsk at apple.com> wrote:
> >
> >
> >> 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.
> >
> > Ah - right, understood. Thanks for explaining!
> >
> > 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.
> >
> > 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.
>
> 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.
>
> 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?
>

Works for me, yeah - thanks!

- Dave

>
> vedant
>
> >
> >
> > 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/832b8692/attachment.html>