This is an archive of the discontinued LLVM Phabricator instance.

Differential D27857

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

Authored by wolfgangp on Dec 16 2016, 11:49 AM.

Details

Reviewers
dblaikie
danielcdh
andreadb
echristo
rob.lougher
probinson
aprantl
Commits
rGce13e716c59a: [DWARF] Null out the debug locs of load instructions that have been moved by…
rL291037: [DWARF] Null out the debug locs of load instructions that have been moved by…
Summary

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 this patch proposes to null out (or not propagate) the debug locations of such instructions for now to improve debugging behavior.

The modified test case phi-translate.ll covers all cases.

Diff Detail

Event Timeline

wolfgangp updated this revision to Diff 81778.Dec 16 2016, 11:49 AM
wolfgangp retitled this revision from to [PATCH][DWARF] Don't propagate or set debug locations for PRE loads and associated address calculations.
wolfgangp updated this object.
wolfgangp added reviewers: aprantl, dblaikie, probinson, danielcdh, andreadb, echristo, rob.lougher.
wolfgangp added a subscriber: llvm-commits.
Herald added a subscriber: mehdi_amini. · View Herald TranscriptDec 16 2016, 11:49 AM
wolfgangp added a comment.Jan 4 2017, 9:04 AM

Ping...

aprantl edited edge metadata.Jan 4 2017, 1:31 PM

Is this a case where using new the merge debug location api would make sense because we are merging multiple loads into one, or are we just deleting it so line table doesn't jump back and forth? If yes, we should use it here, otherwise this looks fine.

wolfgangp added a comment.Jan 4 2017, 2:56 PM
In D27857#635847, @aprantl wrote:

Is this a case where using new the merge debug location api would make sense because we are merging multiple loads into one, or are we just deleting it so line table doesn't jump back and forth? If yes, we should use it here, otherwise this looks fine.

No, we are effectively moving an instruction from its original place to a different BB, so there is nothing to merge. As you said, we are mostly concerned about line table jumping.

aprantl accepted this revision.Jan 4 2017, 3:17 PM
aprantl edited edge metadata.

I have two suggestions for further clarifying the comments. I'm somewhat concerned that this could regress profiling/instrumentation, but since this only affects singular load instructions it shouldn't be too bad.

thanks!

lib/Transforms/Scalar/GVN.cpp
1577

... could lead to confusing (but correct) source attributions.

1612

, and we want to avoid a jumpy line table.

This revision is now accepted and ready to land.Jan 4 2017, 3:17 PM
wolfgangp added a comment.Jan 4 2017, 3:25 PM
In D27857#636122, @aprantl wrote:

I have two suggestions for further clarifying the comments. I'm somewhat concerned that this could regress profiling/instrumentation, but since this only affects singular load instructions it shouldn't be too bad.

thanks!

I'll add them. Thanks for the review.

Closed by commit rL291037: [DWARF] Null out the debug locs of load instructions that have been moved by… (authored by wolfgangp). · Explain WhyJan 4 2017, 4:09 PM
This revision was automatically updated to reflect the committed changes.
aprantl mentioned this in D28390: [DWARF] LICM should null out the debug loc of hoisted loop invariant instructions.Jan 6 2017, 8:37 AM

Revision Contents

PathSize
llvm/
lib/
Transforms/
Scalar/
13 lines
test/
Transforms/
GVN/
PRE/
13 lines

Diff 81778

lib/Transforms/Scalar/GVN.cpp

Context not available.
// Assign value numbers to the new instructions. // Assign value numbers to the new instructions.
for (Instruction *I : NewInsts) { 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.
aprantlUnsubmitted
Not Done
ReplyInline Actions

... could lead to confusing (but correct) source attributions.

aprantl: ... could lead to confusing (but correct) 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 // FIXME: We really _ought_ to insert these value numbers into their
// parent's availability map. However, in doing so, we risk getting into // 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 // ordering issues. If a block hasn't been processed yet, we would be
Context not available.
if (auto *RangeMD = LI->getMetadata(LLVMContext::MD_range)) if (auto *RangeMD = LI->getMetadata(LLVMContext::MD_range))
NewLoad->setMetadata(LLVMContext::MD_range, RangeMD); NewLoad->setMetadata(LLVMContext::MD_range, RangeMD);
// Transfer DebugLoc. // We do not propagate the old load's debug location, because the new
NewLoad->setDebugLoc(LI->getDebugLoc()); // load now lives in a different BB.
aprantlUnsubmitted
Not Done
ReplyInline Actions

, and we want to avoid a jumpy line table.

aprantl: , and we want to avoid a jumpy line table.
// FIXME: How do we retain source locations without causing poor debugging
// behavior?
// Add the newly created load. // Add the newly created load.
ValuesPerBlock.push_back(AvailableValueInBlock::get(UnavailablePred, ValuesPerBlock.push_back(AvailableValueInBlock::get(UnavailablePred,
Context not available.

test/Transforms/GVN/PRE/phi-translate.ll

Context not available.
; CHECK-LABEL: @foo( ; CHECK-LABEL: @foo(
; CHECK: entry.end_crit_edge: ; CHECK: entry.end_crit_edge:
; CHECK: %j.phi.trans.insert = sext i32 %x to i64, !dbg [[J_LOC:![0-9]+]] ; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{$}}
; CHECK: %q.phi.trans.insert = getelementptr {{.*}}, !dbg [[Q_LOC:![0-9]+]] ; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %[[INDEX]]{{$}}
; CHECK: %n.pre = load i32, i32* %q.phi.trans.insert, !dbg [[N_LOC:![0-9]+]] ; CHECK: %n.pre = load i32, i32* %[[ADDRESS]]{{$}}
; CHECK: br label %end
; CHECK: then: ; CHECK: then:
; CHECK: store i32 %z ; CHECK: store i32 %z
; CHECK: end: ; 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: ret i32 %n
; CHECK-DAG: [[J_LOC]] = !DILocation(line: 45, column: 1, scope: !{{.*}}) ; CHECK: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
; CHECK-DAG: [[Q_LOC]] = !DILocation(line: 46, column: 1, scope: !{{.*}})
; CHECK-DAG: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
@G = external global [100 x i32] @G = external global [100 x i32]
define i32 @foo(i32 %x, i32 %z) !dbg !6 { define i32 @foo(i32 %x, i32 %z) !dbg !6 {
Context not available.