Page MenuHomePhabricator

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

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

Function passes that update/preserve MemorySSA only in the old pass manager:

  • 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

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.

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.

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...


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


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.

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.