[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:
> > 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152743