[PATCH] D152743: [AliasAnalysis] Return NoModRef for loads from constant memory

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:482
+  // If the load is from constant memory, it doesn't mod/ref anything.
+  if (!isRefSet(getModRefInfoMask(MemoryLocation::get(L))))
+    return ModRefInfo::NoModRef;
----------------
nikic wrote:
> aeubanks wrote:
> > nikic wrote:
> > > aeubanks wrote:
> > > > aeubanks wrote:
> > > > > asbirlea wrote:
> > > > > > aeubanks wrote:
> > > > > > > nikic wrote:
> > > > > > > > nikic wrote:
> > > > > > > > > Should probably use `Loc` rather than `MemoryLocation::get(L)` here, just like all the other methods?
> > > > > > > > Or I guess not, if the intention is to pick up TBAA metadata from the load itself?
> > > > > > > yeah exactly
> > > > > > I think there's subtle distinction here with the Store case below. For Store, if either `MemoryLocation::get(S)` OR `Loc` are invariable, the answer is still NoModRef, because, well, it's invariable and the store cannot modify any such locations. So the `getModRefInfoMask` check could go before the `alias` check and still be correct.
> > > > > > 
> > > > > > For the load, you should still return Ref when `MemoryLocation::get(L) == Loc`, and that's given by the alias check.
> > > > > > Ref means "this instructions is reading". This is true when the locations are the same (or may have some overlap), it *is* reading  *that* invariant location. It is not true when the locations are different.
> > > > > > So the alias check is needed first, and if that one cannot prove NoAlias, the conservative answer should be Ref: "the Load instruction may be reading Loc, and I don't care that it's invariant, it may still be reading it".
> > > > > > 
> > > > > your response mostly makes sense to me and I'll send out a patch to update MemorySSA instead, but then should [this](https://github.com/llvm/llvm-project/blob/518621e90b0bcdb1c261e06256b9c845ef674cf2/llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp#L402) be returning `Ref` since it can be read from?
> > > > actually the llvm docs seem to be inconsistent
> > > > 
> > > > https://llvm.org/docs/AliasAnalysis.html#the-getmodrefinfomask-method says that globally constant memory is `NoModRef` and locally constant memory is `Ref`, but https://llvm.org/docs/LangRef.html#representation says that `pointsToConstantMemory` should return true for invariant tbaa metadata, but [`pointsToConstantMemory`](https://github.com/llvm/llvm-project/blob/3391bdc255f1a75c59d71c7305959e84d8d5f468/llvm/include/llvm/Analysis/AliasAnalysis.h#L388) is just a wrapper around `isNoModRef(getModRefInfoMask())`
> > > ModRefInfo works in terms of memory effects. A read from globally invariant memory is not observable, and as such NoModRef. You can reorder such a read past arbitrary modifications.
> > > 
> > > Returning NoModRef is fine (from a legality perspective) if either Loc or MemoryLocation::get(L) is globally invariant.
> > > 
> > > pointsToConstantMemory() is a legacy method with conservative behavior -- it should probably be replaced with getModRefInfoMask() that check the effects that are relevant for the transform.
> > but loads with invariant tbaa metadata are not typically globally invariant right? the first doc says that corresponds to `Ref`, but the second doc says that it's ok to return `NoModRef` for this tbaa metadata
> Invariant TBAA metadata requires global invariance, NoModRef is correct.
ah ok, although then I don't know what that'd be used for then if it required global invariance

anyway, https://reviews.llvm.org/D152859 for the MemorySSA change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152743/new/

https://reviews.llvm.org/D152743