Page MenuHomePhabricator

[PM] Port LoopSink to the new pass manager.
ClosedPublic

Authored by chandlerc on Jan 19 2017, 2:34 PM.

Details

Summary

Like several other loop passes (the vectorizer, etc) this pass doesn't
really fit the model of a loop pass. The critical distinction is that it
isn't intended to be pipelined together with other loop passes. I plan
to add some documentation to the loop pass manager to make this more
clear on that side.

LoopSink is also different because it doesn't really need a lot of the
infrastructure of our loop passes. For example, if there aren't loop
invariant instructions causing a preheader to exist, there is no need to
form a preheader. It also doesn't need LCSSA because this pass is
only involved in sinking invariant instructions from a preheader into
the loop, not reasoning about live-outs.

This allows some nice simplifications to the pass in the new PM where we
can directly walk the loops once without restructuring them.

Event Timeline

chandlerc created this revision.Jan 19 2017, 2:34 PM
davide requested changes to this revision.Jan 19 2017, 2:54 PM

The logic mostly looks fine (RPO walk etc..). Some minor comments, and I'm a little bit confused by the fact you don't pass SCEV to this pass anymore. See comments inline.

lib/Transforms/Scalar/LoopSink.cpp
311–326 ↗(On Diff #85091)

This pattern is actually proliferating. How hard is to make this an helper before this patch goes in? (and replace the other uses)

331–332 ↗(On Diff #85091)

The old PM actually passes SCEV to sinkLoopInvariantInstruction. Here you don't, what are the consequences?

340–345 ↗(On Diff #85091)

As far as I can see the old pass doesn't preserve AA passes.
I'm fine with this going in but maybe can be separated?

359–360 ↗(On Diff #85091)

Ugh, this seems wrong (not your fault of course). The transform seems to ask for AA and LoopInfo but doesn't quite list them as required.

This revision now requires changes to proceed.Jan 19 2017, 2:54 PM
sanjoy added inline comments.Jan 19 2017, 2:58 PM
lib/Transforms/Scalar/LoopSink.cpp
317 ↗(On Diff #85091)

Don't you need to traverse LI in reverse? That way you'll push to PreOrderLoops in program order, and pop from it in reverse program order (which is what the pass expects).

345 ↗(On Diff #85091)

Didn't you invalidate SCEV? Can you still preserve SCEVAA?

chandlerc updated this revision to Diff 85056.Jan 19 2017, 4:27 PM
chandlerc edited edge metadata.
chandlerc marked 3 inline comments as done.

Update with fixes to most of the review comments. Factoring one thing out into
a separate patch.

chandlerc marked 2 inline comments as done.Jan 19 2017, 4:28 PM

Most comments addressed. Factoring out the loop walk to a helper.

lib/Transforms/Scalar/LoopSink.cpp
311–326 ↗(On Diff #85091)

Sure. And as Sanjoy spotted, it even has a tiny bug.

331–332 ↗(On Diff #85091)

It is only used to invalidate the loop in SCEV. Now we just don't preserve SCEV (and don't require it either). I'll add a comment.

340–345 ↗(On Diff #85091)

In the new PM, we can actually preserve SCEVAA and not SCEV and it can work! But I don't think its in good shape yet.

And Davide, as you may have noticed below, the getLoopAnalysisUsage makes the old PM preserve AA as well! Yay!

Anyways, I'll just rip out AA preservation for now. We can add it back with intent and clarity later.

359–360 ↗(On Diff #85091)

It does inside of getLoopAnalysisUsage. Yay subtle. =[

sanjoy accepted this revision.Jan 19 2017, 4:29 PM

lgtm

davide accepted this revision.Jan 19 2017, 4:35 PM

Looks good to me, thanks for clarifying =)

This revision is now accepted and ready to land.Jan 19 2017, 4:35 PM
chandlerc updated this revision to Diff 85091.Jan 19 2017, 6:59 PM
chandlerc marked an inline comment as done.

Update to use the newly added helper to compute the preorder walk, and
a freshened comment.

FYI, landing as I think this addressed everything and have LGTMs. But if anything ends up surprising folks, don't hesitate to pipe up!

This revision was automatically updated to reflect the committed changes.