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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

In D152743#4415131 <https://reviews.llvm.org/D152743#4415131>, @nikic wrote:

> I'm concerned that this will cause us problems in the future, e.g. if we want to do MemorySSA-based GVN, because we will no longer have MemoryUses for these loads at all, even though we may want to do load CSE on them.

I think the issue is we can no longer determine when a true read happens.
The change likely needs to go into MSSA, have the isUseTriviallyOptimizableToLiveOnEntry <https://github.com/llvm/llvm-project/blob/1d6c3e29f6aa45914faa7be00a939b8f550f38e9/llvm/lib/Analysis/MemorySSA.cpp#L373> check done during construction outside of optimizeUses, as a mandatory "cheap enough" optimization.
Over there the method is saying "it's true you may be a reading instruction, but you're not going to be modified by anything else anyway so you're getting optimized to LoE". 
GVN still needs to account for two such MemoryUses and their relation to do CSE/PRE.



================
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:
> 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".



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152743