[PATCH] D51718: Update MemorySSA in LoopRotate.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:49
+  bool Changed = LoopRotation(&L, &AR.LI, &AR.TTI, &AR.AC, &AR.DT, &AR.SE,
+                              MSSAU.hasValue() ? MSSAU.getPointer() : nullptr,
+                              SQ, false, Threshold, false);
----------------
greened wrote:
> asbirlea wrote:
> > greened wrote:
> > > Why not just pass the Optional?
> > I'm inclined to prefer using the pointer just due to consistency with the other passes + the hope that this won't be Optional forever :).
> > 
> > If you or other reviewers strongly prefer passing the Optional, please let me know and I'll update here and in all other passes.
> I don't have a strong position on this, just throwing it out as an idea.  Using an optional does eliminate the ternary operations and would make the code a bit cleaner.
> 
> How expensive is MemorySSA?  I'm not sure we can guarantee it will be available so it might always have to be optional.
It can be expensive if not used but it should be cheaper than MemoryDependenceAnalysis the AliasSetTracker.
End goal is to use it in LICM instead of AST, and in BBUtils instead of MemoryDependenceAnalysis. There are probably others, but these are on my list for now.


I tried to go ahead and make the change to Optional, but I end up moving the ternary somewhere else: either in the constructor for LoopRotate, or in each call to a BasicBlockUtil (e.g. MergeBlockIntoPredecessor or CriticalEdgeSplittingOptions).

In utils, it seems the norm is to use pointers and check for null. Not saying using an Optional wouldn't be better, but it will require having all passes changed to pass Optional as well in the same patch.
I think such a refactoring should be separate from this patch.
What do you think?



Repository:
  rL LLVM

https://reviews.llvm.org/D51718