Page MenuHomePhabricator

[MemorySSA] LCSSA preserves MemorySSA.

Authored by asbirlea on Apr 17 2019, 10:29 AM.



Enabling MemorySSA in the old pass manager leads to MemorySSA being run
twice due to the fact that LCSSA and LoopSimplify do not preserve
MemorySSA. This is the first step to address that: target LCSSA.

LCSSA does not make any changes that invalidate MemorySSA, so it
preserves it by design. It must preserve AA as well, for this to hold.

After this patch, MemorySSA is still run twice.
Step two follows: target LoopSimplify.

Diff Detail


Event Timeline

asbirlea created this revision.Apr 17 2019, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 10:29 AM
efriedma added inline comments.
417 ↗(On Diff #195598)

This variable is unused?

asbirlea updated this revision to Diff 195819.Apr 18 2019, 2:59 PM
asbirlea marked an inline comment as done.

Simplify patch to only rpeserve and not require.

asbirlea edited the summary of this revision. (Show Details)Apr 18 2019, 3:00 PM
asbirlea edited the summary of this revision. (Show Details)
asbirlea edited the summary of this revision. (Show Details)Apr 18 2019, 4:11 PM
asbirlea added reviewers: chandlerc, efriedma.
chandlerc requested changes to this revision.Apr 18 2019, 4:38 PM

Mostly questions around the AA manager preservation, see inline.

64–66 ↗(On Diff #195836)

You should move the definition as well as the declaration.

It does make sense to me to put this in MemorySSA.cpp itself -- its something a bunch of users of it need to coordinate about, including the loop analysis manager.

490–491 ↗(On Diff #195836)

Should the AA manager just preserve itself instead so that this isn't so confusing for users?

This revision now requires changes to proceed.Apr 18 2019, 4:38 PM
asbirlea marked an inline comment as done.Apr 18 2019, 4:46 PM
asbirlea added inline comments.
490–491 ↗(On Diff #195836)

Sure? I though it was stateless already, so preserved by default, but I'm unclear how to mark that.

This got me *very* confused in the first iteration of this patch, when I noticed a new AAManager being built for the LoopPassManager. Why would that happen?

chandlerc added inline comments.Apr 18 2019, 11:28 PM
490–491 ↗(On Diff #195836)

It really should be, but it isn't built that way yet. This should probably be a preparatory patch. It'll just involve removing the check for the AAManager itself in the AAResults::invalidate method and adding relevant documentation.

Would be good to very explicitly document that all the AAs need to be registered *before* the AAManager is run in that case.

asbirlea updated this revision to Diff 196140.Apr 22 2019, 3:13 PM
asbirlea marked 3 inline comments as done.

Rebase on top of D60914, remove the need to preserve AAManager.

This revision is now accepted and ready to land.Apr 22 2019, 4:41 PM
This revision was automatically updated to reflect the committed changes.