[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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152743