[PATCH] D27857: [PATCH][DWARF] Don't propagate or set debug locations for PRE loads and associated address calculations

[David Blaikie via llvm-commits]

On Fri, Dec 16, 2016 at 11:49 AM Wolfgang Pieb via Phabricator <
reviews at reviews.llvm.org> wrote:

> wolfgangp created this revision.
> wolfgangp added reviewers: aprantl, dblaikie, probinson, danielcdh,
> andreadb, echristo, rob.lougher.
> wolfgangp added a subscriber: llvm-commits.
> Herald added a subscriber: mehdi_amini.
>
> This is another instance of the 'nulling out the debug location of moved
> instructions' theme. The GVN pass, when performing PRE on load instructions
> retains the instructions' original debug loc, which causes erratic stepping
> behavior and incorrect source attribution for autoFDO purposes.
> Keeping in mind the ongoing debate about Debug locations for optimized
> code <http://lists.llvm.org/pipermail/llvm-dev/2016-December/107847.html>
> this patch proposes to null out (or not propagate) the debug locations of
> such instructions for now to improve debugging behavior.
>

Does this only apply to cases that would be a problem for profile accuracy
- or does it apply in cases where code is moved within a single basic
block? (not sure which kind of moves GVN/PRE do nor which ones your change
is proposing to change the debug info for)


>
> The modified test case phi-translate.ll covers all cases.
>
>
> https://reviews.llvm.org/D27857
>
> Files:
>   lib/Transforms/Scalar/GVN.cpp
>   test/Transforms/GVN/PRE/phi-translate.ll
>
>
> Index: test/Transforms/GVN/PRE/phi-translate.ll
> ===================================================================
> --- test/Transforms/GVN/PRE/phi-translate.ll
> +++ test/Transforms/GVN/PRE/phi-translate.ll
> @@ -4,18 +4,17 @@
>
>  ; CHECK-LABEL: @foo(
>  ; CHECK: entry.end_crit_edge:
> -; CHECK:   %j.phi.trans.insert = sext i32 %x to i64, !dbg
> [[J_LOC:![0-9]+]]
> -; CHECK:   %q.phi.trans.insert = getelementptr {{.*}}, !dbg
> [[Q_LOC:![0-9]+]]
> -; CHECK:   %n.pre = load i32, i32* %q.phi.trans.insert, !dbg
> [[N_LOC:![0-9]+]]
> +; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{$}}
> +; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x
> i32]* @G, i64 0, i64 %[[INDEX]]{{$}}
> +; CHECK:   %n.pre = load i32, i32* %[[ADDRESS]]{{$}}
> +; CHECK: br label %end
>  ; CHECK: then:
>  ; CHECK:   store i32 %z
>  ; CHECK: end:
> -; CHECK:   %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ],
> !dbg [[N_LOC]]
> +; CHECK:   %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ],
> !dbg [[N_LOC:![0-9]+]]
>  ; CHECK:   ret i32 %n
>
> -; CHECK-DAG: [[J_LOC]] = !DILocation(line: 45, column: 1, scope: !{{.*}})
> -; CHECK-DAG: [[Q_LOC]] = !DILocation(line: 46, column: 1, scope: !{{.*}})
> -; CHECK-DAG: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
> +; CHECK: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
>
>  @G = external global [100 x i32]
>  define i32 @foo(i32 %x, i32 %z) !dbg !6 {
> Index: lib/Transforms/Scalar/GVN.cpp
> ===================================================================
> --- lib/Transforms/Scalar/GVN.cpp
> +++ lib/Transforms/Scalar/GVN.cpp
> @@ -1572,6 +1572,13 @@
>
>    // Assign value numbers to the new instructions.
>    for (Instruction *I : NewInsts) {
> +    // Instructions that have been inserted in predecessor(s) to
> materialize
> +    // the load address do not retain their original debug locations.
> Doing
> +    // so could lead to erratic source attributions.
> +    // FIXME: How do we retain source locations without causing poor
> debugging
> +    // behavior?
> +    I->setDebugLoc(DebugLoc());
> +
>      // FIXME: We really _ought_ to insert these value numbers into their
>      // parent's availability map.  However, in doing so, we risk getting
> into
>      // ordering issues.  If a block hasn't been processed yet, we would be
> @@ -1601,8 +1608,10 @@
>      if (auto *RangeMD = LI->getMetadata(LLVMContext::MD_range))
>        NewLoad->setMetadata(LLVMContext::MD_range, RangeMD);
>
> -    // Transfer DebugLoc.
> -    NewLoad->setDebugLoc(LI->getDebugLoc());
> +    // We do not propagate the old load's debug location, because the new
> +    // load now lives in a different BB.
> +    // FIXME: How do we retain source locations without causing poor
> debugging
> +    // behavior?
>
>      // Add the newly created load.
>      ValuesPerBlock.push_back(AvailableValueInBlock::get(UnavailablePred,
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161216/cbe29be8/attachment.html>