This is an archive of the discontinued LLVM Phabricator instance.

[SROA] Preserve DebugLoc when rewriting alloca partitions
ClosedPublic

Authored by gramanas on Jun 27 2018, 5:40 AM.

Details

Summary

When rewriting an alloca partition copy the DL from the
old alloca over the the new one.

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

Diff Detail

Repository
rL LLVM

Event Timeline

gramanas created this revision.Jun 27 2018, 5:40 AM

Is there an existing test for rewriting the alloca, that you could modify? Instead of adding a whole new test file.
Otherwise seems pretty straightforward. This appears to be a common problem, failing to setDebugLoc after creating a new instruction.

vsk added a comment.Jun 27 2018, 7:38 AM

Is there an existing test for rewriting the alloca, that you could modify? Instead of adding a whole new test file.
Otherwise seems pretty straightforward. This appears to be a common problem, failing to setDebugLoc after creating a new instruction.

+ 1 on all points.

lib/Transforms/Scalar/SROA.cpp
4032 ↗(On Diff #153055)

Please end sentences in comments with periods.

gramanas updated this revision to Diff 153117.Jun 27 2018, 10:16 AM

Addressing the comments

gramanas marked an inline comment as done.Jun 27 2018, 10:18 AM
vsk added inline comments.Jun 27 2018, 11:10 AM
test/Transforms/SROA/alignment.ll
2 ↗(On Diff #153117)

It's OK for a change to instcombine to break the test you're adding here, e.g by disabling a combine.

Is there a test you can repurpose which doesn't rely on instcombine to hit the code path you've changed?

gramanas updated this revision to Diff 153288.Jun 28 2018, 4:04 AM
gramanas marked an inline comment as done.

Remove -instcombine from the RUN clause of the test.

gramanas added inline comments.Jun 28 2018, 4:06 AM
test/Transforms/SROA/alignment.ll
2 ↗(On Diff #153117)

Yes, you are absolutely right. For some reason I thought I it was the combination of the two that caused the -check-debugify error.

There are some more tests that without this patch fail to preserve the DL. Should I add DEBUGLOC tests there as well or is one enough?

gramanas retitled this revision from [WIP][SROA] Preserve DebugLoc when rewriting alloca partitions to [SROA] Preserve DebugLoc when rewriting alloca partitions.Jun 28 2018, 7:10 AM
vsk accepted this revision.Jun 28 2018, 11:12 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 28 2018, 11:12 AM
This revision was automatically updated to reflect the committed changes.