This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][LoopVectorize] Preserve DL in generated phi instruction
ClosedPublic

Authored by gramanas on Jun 29 2018, 5:37 AM.

Details

Summary

Similar to rL335904.

When creating phi instructions to resume at the scalar part of the loop,
copy the DebugLoc from the original phi over to the new one.

Based on Greg's report:
https://bugs.llvm.org/show_bug.cgi?id=37955

Diff Detail

Event Timeline

gramanas created this revision.Jun 29 2018, 5:37 AM
gramanas edited the summary of this revision. (Show Details)Jun 29 2018, 5:39 AM
gramanas added a subscriber: gbedwell.
vsk added inline comments.Jun 29 2018, 9:57 AM
test/Transforms/LoopVectorize/calloc.ll
2 ↗(On Diff #153483)

The two RUN lines here are testing substantially different things. If there isn't a simpler file to repurpose as a debug info test, it would be simpler to create a (perhaps more reduced?) test.

10 ↗(On Diff #153483)

Please add a 'CHECK-LABEL: define {{.*}} <function name>' to ensure that subsequent check lines are testing the correct function.

52 ↗(On Diff #153483)

This simply tests that the phi is assigned a debug location, but it doesn't test that it's assigned a correct location. Please tighten the test by checking the line number.

gramanas marked 3 inline comments as done.Jun 30 2018, 5:09 AM
gramanas updated this revision to Diff 153624.Jun 30 2018, 5:10 AM

Address inline comments.

If there isn't a simpler file to repurpose as a debug info test, it would be simpler to create a (perhaps more reduced?) test.

Since this particular fix is about the phi that resumes the scalar part of the loop, I don't think it would be appropriate to create a new reduced test just for this.
It seems like it's a quite common thing to occur in loop-vectorize.

gramanas updated this revision to Diff 153625.Jun 30 2018, 5:26 AM

Formatting

vsk accepted this revision.Jul 2 2018, 10:18 AM

It'd be a good idea rename the test so that readers know what to expect if it fails (say, 'preserve-dbg-loc-and-loop-metadata.ll').

Otherwise, LGTM, thanks!

This revision is now accepted and ready to land.Jul 2 2018, 10:18 AM
This revision was automatically updated to reflect the committed changes.