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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!



================
Comment at: include/llvm/Analysis/MemorySSA.h:264
   /// be rewalked by the walker if necessary.
   /// This really should only be called by tests.
   inline void resetOptimized();
----------------
If we're going to call `resetOptimized()` outside of tests, we should probably remove this bit of the comment.


================
Comment at: include/llvm/Analysis/MemorySSA.h:284
     if (!Optimized) {
+      // FIXME: is this redundant?
+      resetOptimized();
----------------
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.


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


================
Comment at: lib/Analysis/MemorySSA.cpp:240
                                      const Instruction *UseInst,
-                                     AliasAnalysis &AA) {
+                                     AliasAnalysis &AA, bool &FoundMustAlias) {
   Instruction *DefInst = MD->getMemoryInst();
----------------
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.)


================
Comment at: lib/Analysis/MemorySSA.cpp:292
+    return AA.getModRefInfo(DefInst, UseLoc) & MRI_Mod;
+  else {
+    AliasResult AResult = AA.alias(DefLoc.getValue(), UseLoc);
----------------
Nit: no `else` after `if (...) return ...;`


================
Comment at: lib/Analysis/MemorySSA.cpp:303
                                      const MemoryLocOrCall &UseMLOC,
-                                     AliasAnalysis &AA) {
+                                     AliasAnalysis &AA, bool &FoundMustAlias) {
   // FIXME: This is a temporary hack to allow a single instructionClobbersQuery
----------------
Same "please make this part of the return value" nit


================
Comment at: lib/Analysis/MemorySSA.cpp:881
       resetPhiOptznState();
       Result = OptRes.PrimaryClobber.Clobber;
     }
----------------
Do we need to set Q.FoundMustAlias here, as well?


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


https://reviews.llvm.org/D38569