[PATCH] D27857: [PATCH][DWARF] Don't propagate or set debug locations for PRE loads and associated address calculations
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 16 11:51:45 PST 2016
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>
More information about the llvm-commits
mailing list