This is an archive of the discontinued LLVM Phabricator instance.

[PM/LoopUnswitch] When using the new SimpleLoopUnswitch pass, schedule loop-cleanup passes at the beginning of the loop pass pipeline, and re-enqueue loops after even trivial unswitching.
ClosedPublic

Authored by chandlerc on May 26 2018, 4:24 AM.

Details

Summary

This will allow us to much more consistently avoid simplifying code
while doing trivial unswitching. I've also added a test case that
specifically shows effective iteration using this technique.

I've unconditionally updated the new PM as that is always using the
SimpleLoopUnswitch pass, and I've made the pipeline changes for the old
PM conditional on using this new unswitch pass. I added a bunch of
comments to the loop pass pipeline in the old PM to make it more clear
what is going on when reviewing.

Hopefully this will unblock doing *partial* unswitching instead of just
full unswitching.

Depends on D47407.

Diff Detail

Event Timeline

chandlerc created this revision.May 26 2018, 4:24 AM
chandlerc updated this revision to Diff 148739.May 26 2018, 3:38 PM
chandlerc retitled this revision from [PM/LoopUnswitch] Schedule loop-cleanup passes at the beginning of the loop pass pipeline, and re-enqueue loops after even trivial unswitching. to [PM/LoopUnswitch] When using the new SimpleLoopUnswitch pass, schedule loop-cleanup passes at the beginning of the loop pass pipeline, and re-enqueue loops after even trivial unswitching..
chandlerc edited the summary of this revision. (Show Details)
chandlerc added reviewers: fedor.sergeev, reames.

Rebase and teach the old PM to do this as well when using SimpleLoopUnswitch.

I've also now built a Clang with this and run it over the test suite with both old and new PM and with this unswitch pass enabled. No crashes, no failures.

Looks ok to me.

llvm/lib/Passes/PassBuilder.cpp
396–397

Time to update the comment? :)

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
1937–1938

mention that current loop gets cleaned up as well (if cleanup scheduled)?

dmgreen added inline comments.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
340 ↗(On Diff #148739)

Should this comment be moved down next to the loop rotate pass?

chandlerc updated this revision to Diff 148932.May 29 2018, 9:50 AM
chandlerc marked an inline comment as done.

Move the new cleanup pass additions above the loop-rotate specific comment.
This position was the whole point of the other comment I added here....

Thanks for review all! (Waiting on the patch before this in order to land this one.)

llvm/lib/Passes/PassBuilder.cpp
396–397

No? I think this is still true. We still break it into two pipelines. I don't think LoopSimplifyCFG has had enough of SimplifyCFG ported to it to make it effective to re-merge the loop pipelines (yet).

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
340 ↗(On Diff #148739)

I think I even intended to do that. Not sure how this ended up this way. Anyways, well spotted. Fixed. =D

sanjoy accepted this revision.May 29 2018, 2:03 PM

lgtm

This revision is now accepted and ready to land.May 29 2018, 2:03 PM
fedor.sergeev added inline comments.May 29 2018, 3:04 PM
llvm/lib/Passes/PassBuilder.cpp
396–397

It kind of leaves an impression of "either SimplifyCFG or LoopSimplifyCFG", while with this change it is both.

Perhaps:
"These can and should be *removed* (not replaced) as soon as their equivalents are ready as a full replacement"

fedor.sergeev accepted this revision.May 29 2018, 3:05 PM

Thanks all, landing!

llvm/lib/Passes/PassBuilder.cpp
396–397

I've tried to clarify this comment some. We might still need to replace them or adjust the *position* of these loop cleanup passes, but I've tried to make it much more clear what is going on.

If it still isn't making sense, please keep poking and I'm happy to make follow-up commits to get the comment into a shape where it won't lead to confusion.

This revision was automatically updated to reflect the committed changes.
fedor.sergeev added inline comments.Jun 1 2018, 9:32 AM
llvm/lib/Passes/PassBuilder.cpp
396–397

Thanks, it is fine now.