[PATCH] D38569: Expose must/may alias info in MemorySSA.
George Burgess IV via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 7 10:42:07 PST 2018
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
More information about the llvm-commits
mailing list