<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 16, 2016 at 11:49 AM Wolfgang Pieb via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">wolfgangp created this revision.<br class="gmail_msg">
wolfgangp added reviewers: aprantl, dblaikie, probinson, danielcdh, andreadb, echristo, rob.lougher.<br class="gmail_msg">
wolfgangp added a subscriber: llvm-commits.<br class="gmail_msg">
Herald added a subscriber: mehdi_amini.<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg">
Keeping in mind the ongoing debate about Debug locations for optimized code <<a href="http://lists.llvm.org/pipermail/llvm-dev/2016-December/107847.html" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/pipermail/llvm-dev/2016-December/107847.html</a>> this patch proposes to null out (or not propagate) the debug locations of such instructions for now to improve debugging behavior.<br class="gmail_msg"></blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
The modified test case phi-translate.ll covers all cases.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D27857" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27857</a><br class="gmail_msg">
<br class="gmail_msg">
Files:<br class="gmail_msg">
lib/Transforms/Scalar/GVN.cpp<br class="gmail_msg">
test/Transforms/GVN/PRE/phi-translate.ll<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
Index: test/Transforms/GVN/PRE/phi-translate.ll<br class="gmail_msg">
===================================================================<br class="gmail_msg">
--- test/Transforms/GVN/PRE/phi-translate.ll<br class="gmail_msg">
+++ test/Transforms/GVN/PRE/phi-translate.ll<br class="gmail_msg">
@@ -4,18 +4,17 @@<br class="gmail_msg">
<br class="gmail_msg">
; CHECK-LABEL: @foo(<br class="gmail_msg">
; CHECK: entry.end_crit_edge:<br class="gmail_msg">
-; CHECK: %j.phi.trans.insert = sext i32 %x to i64, !dbg [[J_LOC:![0-9]+]]<br class="gmail_msg">
-; CHECK: %q.phi.trans.insert = getelementptr {{.*}}, !dbg [[Q_LOC:![0-9]+]]<br class="gmail_msg">
-; CHECK: %n.pre = load i32, i32* %q.phi.trans.insert, !dbg [[N_LOC:![0-9]+]]<br class="gmail_msg">
+; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{$}}<br class="gmail_msg">
+; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %[[INDEX]]{{$}}<br class="gmail_msg">
+; CHECK: %n.pre = load i32, i32* %[[ADDRESS]]{{$}}<br class="gmail_msg">
+; CHECK: br label %end<br class="gmail_msg">
; CHECK: then:<br class="gmail_msg">
; CHECK: store i32 %z<br class="gmail_msg">
; CHECK: end:<br class="gmail_msg">
-; CHECK: %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ], !dbg [[N_LOC]]<br class="gmail_msg">
+; CHECK: %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ], !dbg [[N_LOC:![0-9]+]]<br class="gmail_msg">
; CHECK: ret i32 %n<br class="gmail_msg">
<br class="gmail_msg">
-; CHECK-DAG: [[J_LOC]] = !DILocation(line: 45, column: 1, scope: !{{.*}})<br class="gmail_msg">
-; CHECK-DAG: [[Q_LOC]] = !DILocation(line: 46, column: 1, scope: !{{.*}})<br class="gmail_msg">
-; CHECK-DAG: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})<br class="gmail_msg">
+; CHECK: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})<br class="gmail_msg">
<br class="gmail_msg">
@G = external global [100 x i32]<br class="gmail_msg">
define i32 @foo(i32 %x, i32 %z) !dbg !6 {<br class="gmail_msg">
Index: lib/Transforms/Scalar/GVN.cpp<br class="gmail_msg">
===================================================================<br class="gmail_msg">
--- lib/Transforms/Scalar/GVN.cpp<br class="gmail_msg">
+++ lib/Transforms/Scalar/GVN.cpp<br class="gmail_msg">
@@ -1572,6 +1572,13 @@<br class="gmail_msg">
<br class="gmail_msg">
// Assign value numbers to the new instructions.<br class="gmail_msg">
for (Instruction *I : NewInsts) {<br class="gmail_msg">
+ // Instructions that have been inserted in predecessor(s) to materialize<br class="gmail_msg">
+ // the load address do not retain their original debug locations. Doing<br class="gmail_msg">
+ // so could lead to erratic source attributions.<br class="gmail_msg">
+ // FIXME: How do we retain source locations without causing poor debugging<br class="gmail_msg">
+ // behavior?<br class="gmail_msg">
+ I->setDebugLoc(DebugLoc());<br class="gmail_msg">
+<br class="gmail_msg">
// FIXME: We really _ought_ to insert these value numbers into their<br class="gmail_msg">
// parent's availability map. However, in doing so, we risk getting into<br class="gmail_msg">
// ordering issues. If a block hasn't been processed yet, we would be<br class="gmail_msg">
@@ -1601,8 +1608,10 @@<br class="gmail_msg">
if (auto *RangeMD = LI->getMetadata(LLVMContext::MD_range))<br class="gmail_msg">
NewLoad->setMetadata(LLVMContext::MD_range, RangeMD);<br class="gmail_msg">
<br class="gmail_msg">
- // Transfer DebugLoc.<br class="gmail_msg">
- NewLoad->setDebugLoc(LI->getDebugLoc());<br class="gmail_msg">
+ // We do not propagate the old load's debug location, because the new<br class="gmail_msg">
+ // load now lives in a different BB.<br class="gmail_msg">
+ // FIXME: How do we retain source locations without causing poor debugging<br class="gmail_msg">
+ // behavior?<br class="gmail_msg">
<br class="gmail_msg">
// Add the newly created load.<br class="gmail_msg">
ValuesPerBlock.push_back(AvailableValueInBlock::get(UnavailablePred,<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>