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

[George Burgess IV via llvm-commits]

(...Please ignore the bug in my helper where I said getDefiningAccess()
instead of getOptimized(), assuming what we want is the optimized
access :) )

On Wed, Oct 4, 2017 at 8:09 PM, George Burgess IV via Phabricator
<reviews at reviews.llvm.org> wrote:
> 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
>
>
>