This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Merge DebugLoc when speculatively hoisting store instruction
ClosedPublic

Authored by twoh on Jan 23 2017, 5:38 PM.

Details

Summary

Along with https://reviews.llvm.org/D27804, debug locations need to be merged when hoisting store instructions as well. Not sure if just dropping debug locations would make more sense for this case, but as the branch instruction will have at least different discriminator with the hoisted store instruction, I think there will be no difference in practice.

Event Timeline

twoh created this revision.Jan 23 2017, 5:38 PM
aprantl accepted this revision.Jan 23 2017, 8:34 PM

Code LGTM, testcase could be improved a bit (comment inline).

test/Transforms/SimplifyCFG/remove-debug-2.ll
4

These CHECKs could be improved by adding a little more context so it is more obvious what is being checked. This also makes it easier to update the test in the future.

This revision is now accepted and ready to land.Jan 23 2017, 8:34 PM
twoh updated this revision to Diff 85526.Jan 23 2017, 8:59 PM

Address comments on test from @aprantl. Thanks for the comments. Wonder if it looks better now.

twoh added a comment.Jan 26 2017, 7:08 PM

Friendly ping @aprantl. If the new test looks good to you, I'll commit this patch. Thanks!

aprantl added inline comments.Jan 27 2017, 9:05 PM
test/Transforms/SimplifyCFG/remove-debug-2.ll
4

Sorry for the delay!
This is an improvement, but not what I had in mind. Typically, we check for a specific instruction and then match it up with the DILocation, so we can be sure that the location is attached to what we think it is.

e.g.

CHECK: define @bar
CHECK-NOT: ret
CHECK: void call @foo, !dbg ![[LOC1:[0-9]+]]

CHECK: ![[LOC1]] = DILocation(line: 2

PS: feel free to commit at any time after fixing this.

twoh updated this revision to Diff 86165.Jan 27 2017, 11:09 PM

Addressing comments from @aprantl. Thanks for the details comments!

twoh closed this revision.Jan 27 2017, 11:16 PM