[PATCH] D38569: Expose must/may alias info in MemorySSA.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Overall approach looks good to me. Just a few naming/refactoring nits and I'm happy to stamp this. Sorry for taking so long to get back to it



================
Comment at: include/llvm/Analysis/MemorySSA.h:296
   Instruction *MemoryInst;
+  bool isMustAlias;
 };
----------------
george.burgess.iv wrote:
> Nit: Can we pack this in a `PtrIntPair` with `MemoryInst`?
ping :)


================
Comment at: include/llvm/Analysis/MemorySSA.h:258
 
+  AliasResult definingAccessType() const { return AliasWithDefining; }
+
----------------
Is this the type of the defining access, or of the optimized access? (They're identical for Uses, but not for Defs) I assume it's the latter, but if not, the rest of this comment is just noise. :)

In a perfect world, I think the right thing here would be to hide this and have the walker return it as part of `getClobberingMemoryAccess()`. I also realize that we have workarounds in place because MSSA's walker takes ~forever in some cases, so this 'ideal' API probably won't work so well yet. :(

So, as a compromise, might it be good to rename this `getOptimizedAccessType()`, with a comment that it might disappear and become part of the walker's return value instead?


================
Comment at: include/llvm/Analysis/MemorySSA.h:275
 
-  void setDefiningAccess(MemoryAccess *DMA, bool Optimized = false) {
+  void setDefiningAccessType(AliasResult AR) { AliasWithDefining = AR; }
+
----------------
Continuing with my assumption that `AliasWithDefining` is talking about the optimized access, can we rename to `setOptimizedAccessType`?


================
Comment at: lib/Analysis/MemorySSA.cpp:255
+      AR = AA.alias(MemoryLocation(II->getArgOperand(1)), UseLoc);
+      return {(AR == MustAlias), AR};
     case Intrinsic::lifetime_end:
----------------
Nit: parens are redundant


================
Comment at: lib/Analysis/MemorySSA.cpp:509
+          return {MD, true, MustAlias};
+        else {
+          ClobberAlias CA =
----------------
Nit: no `else` after a `return`


================
Comment at: lib/Analysis/MemorySSA.cpp:2074
+  // Note: Currently, we store the optimized def result in
+  // a separate field, since we can't use the defining access.
   if (auto *MUD = dyn_cast<MemoryUseOrDef>(StartingAccess))
----------------
Nit: reflow this comment to 80-cols, please. (In vim, `gq` will do what you want. Otherwise, joining the lines + running clang-tidy should split it up properly.)


================
Comment at: lib/Analysis/MemorySSA.cpp:2116
+      MUD->setOptimized(DefiningAccess);
+      MUD->setDefiningToMustAlias();
+    }
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > dberlin wrote:
> > > I'm a little concerned about saying we must-alias the live on entry def.
> > > It's abstractly true, but i wonder if stuff will get confusingly broken.
> > > 
> > > LiveOnEntry is really equivalent to "something before this function" not "one specific thing".
> > > 
> > > Thoughts?
> > I was thinking this could be solved by using "Clobber" in the name instead of "Alias", but I don't know if "Clobber" would imply something else that's subtly wrong.
> > 
> > That said, it may be a better idea to just have passes that don't care about MustAlias vs LiveOnEntry to roll their own helper:
> > 
> > ```
> > bool isMustAlias(const MemoryAccess *MA) {
> >   return MA->definingIsMustAlias() || MA->getDefiningAccess() == MSSA->getLiveOnEntryDef();
> > }
> > ```
> > 
> > ...So we can have `isMustAlias` just mean `isMustAlias` :)
> Right now definingIsMayAlias and definingIsMustAlias are opposites. Since an optimized access skipped over NoAlias.
> For optimized accesses May means "I found the defining location and it may alias this access."
> For unoptimized accesses, the default is May.
> 
> I was planning to couple isOptimized() and definingIsMustAlias() to determine if a May alias was found.
> 
> Must with LOE is equivalent to "The defining access is not a MayAlias".
> Perhaps I should only 
> * keep the definingIsMayAlias() accessor
> * setDefiningToMayAlias(bool Arg) setter //Arg=false means Must or LOE
> Thoughts?
(So... I had "That WFM. If we decide to go that way, please also add a comment explaining what it means if `definingIsMayAlias` returns false." that I never submitted. Is this thread still relevant? I don't see `definingIsMayAlias` anywhere but here...)


================
Comment at: unittests/Analysis/MemorySSA.cpp:1067
+  unsigned I = 0;
+  for (StoreInst *V : {SA1, SB1, SC1, SA2, SB2, SC2, SB3}) {
+    MemoryDef *MemDef = dyn_cast_or_null<MemoryDef>(MSSA.getMemoryAccess(V));
----------------
Nit: it looks like (?) we're iterating over the same list in the same order in the other two loops. Can we pop it in a local array or initializer_list or something, then use that as the container for these loops? 


Repository:
  rL LLVM

https://reviews.llvm.org/D38569