[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;
----------------
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())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152743