[PATCH] D116660: [LoopFlatten] Update MemorySSA state

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea accepted this revision.
asbirlea added a comment.
This revision is now accepted and ready to land.

Changes LGTM.
Nit: clang-format.
A simpler way to check all verification is now to pass `VerificationLevel::Full` to `verifyMemorySSA()`; I'm mentioning this for your local testing, in case your build doesn't have EXPENSIVE_CHECKS on already, not to include in this patch.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:824
 
+  Optional<MemorySSAUpdater> MSSAU;
+   if (AR.MSSA) {
----------------
aeubanks wrote:
> fhahn wrote:
> > SjoerdMeijer wrote:
> > > SjoerdMeijer wrote:
> > > > fhahn wrote:
> > > > > Could `MSSAU` be created unconditionally like with the legacy pass manager?
> > > > Thanks for taking a look.
> > > > 
> > > > I borrowed this boilerplate code from SimpleLoopUnswitch, but sure, I guess so and will add that.
> > > > 
> > > Hmm, I see this pattern used in most passes, like LoopRotation, LoopSimplifyCFG (SimpleLoopUnswitch).
> > > 
> > > For my understanding, what would be the benefit of doing this unconditionally?
> > slightly less & more straight-forward code?
> with the legacy PM it looks like we're unconditionally requesting MSSA (constructing it if it doesn't yet exist) and then updating it, which is wasted work if MSSA wasn't already available. we only want to keep MSSA up to date if it's already cached
Nit: I'd stick with the `Optional<MemorySSAUpdater> MSSAU;` to avoid the object allocation when MSSA is not present. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116660/new/

https://reviews.llvm.org/D116660