[PATCH] D51718: Update MemorySSA in LoopRotate.

[David Greene via Phabricator via llvm-commits]

greened added a comment.

LGTM, but I don't know if I should be the one to sign off on this since I've never touched LoopRotate.



================
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);
----------------
asbirlea wrote:
> 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?
> 
Yeah, that sounds fine to me.  I'm fine with the way this is.  Was just wondering if Optional might make it cleaner, but it sounds like that would overly complicate this patch and is better left for a later change.  Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D51718