Page MenuHomePhabricator

[PM] Teach LoopUnroll to update the LPM infrastructure as it unrolls loops.
ClosedPublic

Authored by chandlerc on Jan 18 2017, 12:07 AM.

Details

Summary

We do this by reconstructing the newly added loops after the unroll
completes to avoid threading pass manager details through all the mess
of the unrolling infrastructure.

I've enabled some extra assertions in the LPM to try and catch issues
here and enabled a bunch of unroller tests to try and make sure this is
sane.

Currently, I'm manually running loop-simplify when needed. That should
go away once it is folded into the LPM infrastructure.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Jan 18 2017, 12:07 AM
davide added a subscriber: davide.Jan 18 2017, 12:33 AM
chandlerc updated this revision to Diff 84953.Jan 19 2017, 3:33 AM

Add some detailed comments about what is happening here. After a long
discussion with Mikhail Zolotukhin on IRC, we're both pretty sure this is in
fact the correct logic, but it is sufficiently non-obvious to be worth walking
the reader through with some precision.

Also, I've added a test case that *specifically* triggers the interesting cases
here and checks that we visit the expected loops after both full and partial
unrolling.

I tried to add a test case for loop peeling but currently we don't have any
support for peeling an outer loop.

chandlerc updated this revision to Diff 85230.Jan 20 2017, 9:39 PM

Rebase and ping.

chandlerc updated this revision to Diff 85231.Jan 20 2017, 9:43 PM

Adapt to not run loop-simplify unnecessarily now that it is run automatically.

chandlerc updated this revision to Diff 85232.Jan 20 2017, 9:46 PM

::sigh:: *Actually* remove the unnecessary loop-simplify runs.

davide added inline comments.Jan 23 2017, 9:26 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
116–125 ↗(On Diff #85232)

This is a little verbose (maybe part of this can be moved to a comment? (up to you, I don't feel strong about it).

1157–1159 ↗(On Diff #85232)

#ifdef NDEBUG, maybe ?

chandlerc marked 2 inline comments as done.Jan 23 2017, 10:11 PM

Thanks, both addressed!

lib/Transforms/Scalar/LoopUnrollPass.cpp
116–125 ↗(On Diff #85232)

Nah, it was wordy. Thanks for the suggestion. I both tightened it up and moved part of it to a comment.

chandlerc updated this revision to Diff 85533.Jan 23 2017, 10:11 PM
chandlerc marked an inline comment as done.

Update the patch based on review feedback.

mzolotukhin accepted this revision.Jan 24 2017, 12:03 PM

LGTM with one question inline.

Michael

lib/Transforms/Scalar/LoopPassManager.cpp
47–49 ↗(On Diff #85533)

AFAIR, we have lcssa-verification pass now (see rL285394). Aren't we redoing it here?

I don't know if we want to keep that pass with the new pass manager, but still we probably want to have one or the other.

This revision is now accepted and ready to land.Jan 24 2017, 12:03 PM

Thanks, landing!

lib/Transforms/Scalar/LoopPassManager.cpp
47–49 ↗(On Diff #85533)

I feel like a non-pass solution is more clean with the new PM, but I'm completely open to alternatives. The reason why is because the loop PM is now *directly* forming and maintaining LCSSA rather than relying on cross-pass dependencies. That (IMO) makes it much more reasonable to have the PM itself *enforce* LCSSA.

But maybe there are reasons to prefer the pass approach I don't see, so I'm happy to revisit as necessary.

This revision was automatically updated to reflect the committed changes.