Page MenuHomePhabricator

[LoopFlatten] Move it from LPM2 to LPM1

Authored by SjoerdMeijer on Jan 4 2022, 11:29 AM.



In D110057 we moved LoopFlatten to a LoopPassManager. This caused a regression for our 64-bit targets (the 32-bit were unaffected), the pass is no longer triggering for a motivating example. Long story short, the IR is just very different than expected; we try to match loop statements and particular uses of induction variables. The easiest is to just move LoopFlatten to a place in the pipeline where the IR is as expected, which is just before IndVarSimplify. This means we move it from LPM2 to LPM1, so that it actually runs just a bit earlier from where it was running before. IndVarSimplify is responsible for significant rewrites that are difficult to "look through" in LoopFlatten.

One thing I had to do, and am unsure about, is the necessary change to add MemorySSAAnalysis as preserved. Without this, I am running in this assertion failure:

fatal error: error in backend: Loop pass manager using MemorySSA contains a pass that does not preserve MemorySSA

It looks correct to me to add MemorySSA as preserved, but am still investigating this, and thought about sharing this in the mean time.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Jan 4 2022, 11:29 AM
SjoerdMeijer requested review of this revision.Jan 4 2022, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 11:29 AM

Does LoopFlatten not make use of SCEV?

Does LoopFlatten not make use of SCEV?

I was expecting this question. :-)

But yes, it does. In a nutshell, we discover the induction phi, loop increment, the loop test statement, and verify with SCEV that we have found the right ones. In addition, we only allow particular uses of the induction variable for the transformation to be valid. The latter is where IndVarSimplify is causing troubles; it introduces extra uses of the IV that are unexpected for LoopFlatten. Moving LoopFlatten earlier solves this problem, and I don't see any disadvantages of doing that.

nikic added a comment.Jan 4 2022, 11:47 AM

Could you please add a PhaseOrdering test that demonstrates the issue?

828 ↗(On Diff #397354)

This is very likely not correct without additional changes. This pass changes control flow, and that usually requires updates to MSSA as well.

Thanks for the pointers! I have added a PhaseOrdering test, while I now look into updating MSSA.

I decided to do the updating of MSSA separately in D116660 (and will remove it from here).

@dmgreen : You reverted "rG86825fc2fb36: [LoopFlatten] Move it to a LoopPassManager" because of a downstream regression. With D116660 now accepted/committed, that unblocks this change. This should address this regression, so are you happy with this?

I'm happy so long as the performance remains OK! Does this need to be rebased now?

I'm happy so long as the performance remains OK! Does this need to be rebased now?

Ah, yeah, this needs a bit of rebasing. So I will do that and also double check performance before landing this, and will recommit D110057 and apply this change in one go.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 19 2022, 6:38 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.