[llvm] 9649c20 - [InstCombine] Drop debug loc in TryToSinkInstruction (reland)
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 13 14:15:31 PDT 2020
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
> <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.
>
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.
>
> 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/a9ffa06b/attachment.html>
More information about the llvm-commits
mailing list