[PATCH] D24632: Preserve the debug location when sinking compare instructions

[David Blaikie via llvm-commits]

On Thu, Sep 15, 2016 at 3:31 PM Dehao Chen <danielcdh at gmail.com> wrote:

> For AutoFDO, we want to remove(or set 0-line) debug info for the
> instructions that could be executed speculatively (i.e. instruction gets
> more count that it should). For this patch, it's more like code cloning
> without speculation. Basically the cloned code will split the profile
> counts among different copies, so its effect will be: make the profiled
> count smaller than it should have.
>

Fair enough - makes sense. Thanks for the explanation.


> When we post-process samples, we intend to use the max_count we get from
> all cloned instructions (well, technically not exactly "max", there is some
> voting to prevent from speculated case). As a result, we don't want to lose
> debug info for clones (as soon as it's not speculated clones) because it
> gives us better chance to capture the "max".
>
> The same applies to debugging. We don't want to see gdb go to speculated
> source locations as we don't expect it getting executed. But for clones, as
> soon as it's actually logically executed, we want to have the source
> location reserved.
>
> I'm wondering what's the source code that has been affected by this.
>
> Dehao
>
> On Thu, Sep 15, 2016 at 2:56 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> This seems like the opposite of the other recent change to remove/zero out
> the debug location of an instruction being commoned into a preceeding basic
> block.
>
> Adding Dehao.
>
> Wouldn't this hurt profile based optimizations by attributing code passing
> through other.bb to the entry? Making the Entry seem more common than it
> is?
>
> On Thu, Sep 15, 2016 at 2:52 PM Wolfgang Pieb <wolfgang.pieb at sony.com>
> wrote:
>
> wolfgangp created this revision.
> wolfgangp added reviewers: aprantl, dblaikie.
> wolfgangp added subscribers: llvm-commits, andreadb.
>
> When the CodeGenPrepare pass sinks a compare instruction into the basic
> block of a user, it should preserve its debug location. Not doing so
> negatively affects source line attribution for debugging and AutoFDO.
>
> Patch by Andrea Di Biagio
>
> https://reviews.llvm.org/D24632
>
> Files:
>   lib/CodeGen/CodeGenPrepare.cpp
>   test/DebugInfo/Generic/sunk-compare.ll
>
> Index: test/DebugInfo/Generic/sunk-compare.ll
> ===================================================================
> --- test/DebugInfo/Generic/sunk-compare.ll
> +++ test/DebugInfo/Generic/sunk-compare.ll
> @@ -0,0 +1,46 @@
> +; RUN: opt -S -codegenprepare < %s | FileCheck %s
> +;
> +; This test case has been generated by hand but is inspired by the
> +; observation that compares that are sunk into the basic blocks where
> +; their results are used did not retain their debug locs. This caused
> +; sample profiling to attribute code to the wrong source lines.
> +;
> +; We check that the compare instruction retains its debug loc after
> +; it is sunk into other.bb by the codegen prepare pass.
> +;
> +; CHECK:       other.bb:
> +; CHECK-NEXT:  icmp{{.*}}%x, 0, !dbg ![[MDHANDLE:[0-9]*]]
> +; CHECK:       ![[MDHANDLE]] = !DILocation(line: 2
> +;
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +
> +define i32 @_Z3fooii(i32 %x, i32 %y) !dbg !5 {
> +entry:
> +  %cmp17 = icmp sgt i32 %x, 0, !dbg !6
> +  br label %other.bb, !dbg !6
> +
> +other.bb:
> +  br i1 %cmp17, label %exit1.bb, label %exit2.bb, !dbg !7
> +
> +exit1.bb:
> +  %0 = add i32 %y, 42, !dbg !8
> +  ret i32 %0, !dbg !8
> +
> +exit2.bb:
> +  ret i32 44, !dbg !9
> +
> +}
> +
> +!llvm.dbg.cu = !{!0}
> +!llvm.module.flags = !{!3, !4}
> +
> +!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer:
> "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug,
> enums: !2, globals: !2)
> +!1 = !DIFile(filename: "test.cpp", directory: "/debuginfo/bug/cgp")
> +!2 = !{}
> +!3 = !{i32 2, !"Dwarf Version", i32 4}
> +!4 = !{i32 2, !"Debug Info Version", i32 3}
> +!5 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1,
> file: !1, line: 1, isLocal: false, isDefinition: true, scopeLine: 1, flags:
> DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
> +!6 = !DILocation(line: 2, column: 0, scope: !5)
> +!7 = !DILocation(line: 3, column: 0, scope: !5)
> +!8 = !DILocation(line: 4, column: 0, scope: !5)
> +!9 = !DILocation(line: 5, column: 0, scope: !5)
> Index: lib/CodeGen/CodeGenPrepare.cpp
> ===================================================================
> --- lib/CodeGen/CodeGenPrepare.cpp
> +++ lib/CodeGen/CodeGenPrepare.cpp
> @@ -926,6 +926,8 @@
>        InsertedCmp =
>            CmpInst::Create(CI->getOpcode(), CI->getPredicate(),
>                            CI->getOperand(0), CI->getOperand(1), "",
> &*InsertPt);
> +      // Propagate the debug info.
> +      InsertedCmp->setDebugLoc(CI->getDebugLoc());
>      }
>
>      // Replace a use of the cmp with a use of the new cmp.
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160915/583255fa/attachment.html>