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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

A bundle of nitpicks for you. LGTM assuming those get addressed.

Thanks for working on this!



================
Comment at: include/llvm/Analysis/MemorySSA.h:310
+
+  int OptionalAliasResultToThreeBitInt(Optional<AliasResult> OAR) {
+    if (OAR == None)
----------------
nit: newFunctionsShouldStartWithALowerCaseLetter :)


================
Comment at: include/llvm/Analysis/MemorySSA.h:310
+
+  int OptionalAliasResultToThreeBitInt(Optional<AliasResult> OAR) {
+    if (OAR == None)
----------------
george.burgess.iv wrote:
> nit: newFunctionsShouldStartWithALowerCaseLetter :)
nit: `static`?


================
Comment at: include/llvm/Analysis/MemorySSA.h:313
+      return 4;
+    return (int)OAR.getValue();
+  }
----------------
Nit: `(int)OAR.getValueOr(4);`? Or does that require awkward casting? If it does need additional casting, I'm happy with this as-is.


================
Comment at: include/llvm/Analysis/MemorySSA.h:316
+
+  Optional<AliasResult> ThreeBitIntToOptionalAliasResult(int I) const {
+    assert((I <= 7 && I >= 0) &&
----------------
Same casing + `static` nits


================
Comment at: include/llvm/Analysis/MemorySSA.h:317
+  Optional<AliasResult> ThreeBitIntToOptionalAliasResult(int I) const {
+    assert((I <= 7 && I >= 0) &&
+           "Invalid value for converting to an Optional<AliasResult>");
----------------
Should this accept 6 and 7? I thought the values were {May,Must,Partial,No}Alias (for 0-3) and None (for 4)


================
Comment at: include/llvm/Analysis/MemorySSA.h:319
+           "Invalid value for converting to an Optional<AliasResult>");
+    if(I >= 4)
+      return None;
----------------
Nit: `if (`


================
Comment at: lib/Analysis/MemorySSA.cpp:2116
+      MUD->setOptimized(DefiningAccess);
+      MUD->setDefiningToMustAlias();
+    }
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > 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...)
> I used dberlin's advice to "use the AliasResult type", so the only accessor now is definingAccessType() renamed to getOptimizedAccessType(). This returns an AliasResult. The setter also gets an AliasResult.
> Per offline discussion, updated to use an Optional<AliasResult>, where None is used when the optimized access is LiveOnEntry. 
SGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D38569