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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked an inline comment as done.
asbirlea added a comment.

Updates - part1.



================
Comment at: include/llvm/Analysis/MemorySSA.h:284
     if (!Optimized) {
+      // FIXME: is this redundant?
+      resetOptimized();
----------------
george.burgess.iv wrote:
> Should be. In `isOptimized()`, we check to see if `OptimizedID` is the ID of the defining access. If we're setting the operand to something else, we should observe a different ID.
> 
> IIRC, this was necessary to make things like RAUW work without us having to manually deoptimize everything. If that's broken somehow, we shouldn't be papering over it here.
Got it! Removing this, keeping above comment.


================
Comment at: lib/Analysis/MemorySSA.cpp:240
                                      const Instruction *UseInst,
-                                     AliasAnalysis &AA) {
+                                     AliasAnalysis &AA, bool &FoundMustAlias) {
   Instruction *DefInst = MD->getMemoryInst();
----------------
george.burgess.iv wrote:
> I'd mildly prefer that we return a `struct { bool IsClobber, IsMustAlias; };` or similar from this instead. That way, it's more clear what we want to hand back for `FoundMustAlias` in all cases.
> 
> (Moreover, there are quite a few paths in this function that don't try to set `FoundMustAlias`. It doesn't seem that's intentional.)
I did not mean to set it on all paths, since FoundMustAlias should not be modified for NoAlias. I missed the case when it needs to reset to May.

Updating to return pair, where isMustAlias should be ignored if !IsClobber.


================
Comment at: lib/Analysis/MemorySSA.cpp:269
   if (UseCS) {
     ModRefInfo I = AA.getModRefInfo(DefInst, UseCS);
     return I != MRI_NoModRef;
----------------
dberlin wrote:
> It is possible for a call to must-alias a single thing, no?
> 
That's true. I didn't add that because I didn't see how we would use the must info here.

If we know the call does not modify and it's a must alias, then we could consider some transformation (read: promotion). In practice, we won't promote calls, so keeping FoundMustAlias = false (i.e. may) should be conservative.
The right thing is reseting to may for calls: 
if(I != MRI_NoModRef) FoundMustAlias = false;
(instead of keeping the given value).


================
Comment at: lib/Analysis/MemorySSA.cpp:297
+    // Must return if location is modified, not AResult != NoAlias;
+    return AA.getModRefInfo(DefInst, UseLoc) & MRI_Mod;
+  }
----------------
dberlin wrote:
> So this is going to double the number of alias calls, unfortunately. I suspect it will make building significantly slower, since roughly *all* time is spent there.
> 
> Do you have perf numbers to say one way or the other?
> 
> (unfortunately, ->alias is not const, so it will call it again)
> I think we may need to finally implement some small LRU aliasing cache or something to make this fast enough.
> 
> Or 
> 1.  Optionally pass the aliasing result into getModRefInfo, which would be a really weird API).
> 2. Optionally return the aliasing result from getModRefInfo, which may make more sense.
> 
> 
Ack, looking into how to best address this one.


================
Comment at: lib/Analysis/MemorySSA.cpp:881
       resetPhiOptznState();
       Result = OptRes.PrimaryClobber.Clobber;
     }
----------------
george.burgess.iv wrote:
> Do we need to set Q.FoundMustAlias here, as well?
Intended to keep the default (=false) for Phis.


================
Comment at: lib/Analysis/MemorySSA.cpp:2116
+      MUD->setOptimized(DefiningAccess);
+      MUD->setDefiningToMustAlias();
+    }
----------------
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?


https://reviews.llvm.org/D38569