Page MenuHomePhabricator

[MemorySSA & LoopPassManager] Enable MemorySSA as loop dependency. Update tests.

Authored by asbirlea on Feb 15 2019, 3:34 PM.



This patch enables the use of MemorySSA in the old and new loop pass managers.

Passes that currently use MemorySSA:

  • EarlyCSE

Passes that use MemorySSA after this patch:

  • EarlyCSE
  • LICM
  • SimpleLoopUnswitch

Loop passes that update MemorySSA (and do not use it yet, but could use it after this patch):

  • LoopInstSimplify
  • LoopSimplifyCFG
  • LoopUnswitch
  • LoopRotate
  • LoopSimplify

Loop passes that do *not* update MemorySSA:

  • IndVarSimplify
  • LoopDelete
  • LoopIdiom
  • LoopSink
  • LoopUnroll
  • LoopInterchange
  • LoopUnrollAndJam
  • LoopVectorize
  • LoopReroll
  • IRCE

Diff Detail


Event Timeline

asbirlea created this revision.Feb 15 2019, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 3:34 PM
asbirlea updated this revision to Diff 191570.Mar 20 2019, 1:31 PM

Rebase and ping for comments.

chandlerc added inline comments.Mar 20 2019, 6:46 PM
160 ↗(On Diff #191570)

This is weird.

If simplify loops and LCSSA aren't using MemorySSA we shouldn't be building it here only to trash it and rebuild it below. This will basically double the compile time cost (from building).

asbirlea marked an inline comment as done.Mar 28 2019, 4:51 PM
asbirlea added inline comments.
160 ↗(On Diff #191570)

To clarify this in writing, yes, currently MemorySSA is built twice in the old pass manager, due to the order of analysis. LoopSimplify and LCSSA need to preserve MemorySSA to resolve this.

The new pass manager does not have this issue.

sanjoy removed a reviewer: sanjoy.Apr 1 2019, 11:08 AM
asbirlea marked 2 inline comments as done.
asbirlea added inline comments.
160 ↗(On Diff #191570)

Added dependent revisions that update LCSSA and LoopSimplify. PTAL.

asbirlea updated this revision to Diff 195834.Apr 18 2019, 4:06 PM

Rebase on top of patches updating LCSSA and LoopSimplify.

asbirlea updated this revision to Diff 197894.May 2 2019, 5:02 PM

Rebase on ToT and D60833.

One question about a test change and a minor nit pick on comments...

32 ↗(On Diff #195834)

Is this actually an additional load? Or was it always there before but the test just didn't have a CHECK line for it?

96 ↗(On Diff #197894)

No need for the extra comment here IMO... Comments should just document the current state, not transitions.

asbirlea marked 3 inline comments as done.May 6 2019, 9:28 PM
asbirlea added inline comments.
32 ↗(On Diff #195834)

It's not an additional load, it was always there and there was no CHECK line.
The test difference is that the store is hoisted instead of sunk.

asbirlea updated this revision to Diff 198394.May 6 2019, 9:29 PM
asbirlea marked an inline comment as done.

Update comment in test.

asbirlea updated this revision to Diff 212240.Jul 29 2019, 3:30 PM

Rebase on ToT.
Ready for review.

asbirlea updated this revision to Diff 213394.Aug 5 2019, 10:06 AM

Reabse and gentle ping.

asbirlea edited the summary of this revision. (Show Details)Aug 13 2019, 10:42 AM

As we discussed in person, we should refactor this so that when we enable MemorySSA we actually check the the loop passes in question mnanage to preserve it.

173–178 ↗(On Diff #213394)

I think you can just remove this for now.

asbirlea updated this revision to Diff 215726.Aug 16 2019, 5:54 PM
asbirlea marked an inline comment as done.

Rebase on top of D66376.

Gentle ping.

chandlerc accepted this revision.Aug 28 2019, 4:22 PM

FWIW, LGTM. Thanks for all the work figuring out the right way to manage this at each step, and seeing it through, I think this is really great.

As I mentioned in person, good to send a "heads up" FYI email to llvm-dev (on a fresh thread) so folks are aware this is landing, but then I think landing this and updating the release notes are great. I'm really excited to see this come together.

This revision is now accepted and ready to land.Aug 28 2019, 4:22 PM

Thanks Chandler!
Heads up sent to llvm-dev.

This revision was automatically updated to reflect the committed changes.