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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D152743#4415526 <https://reviews.llvm.org/D152743#4415526>, @asbirlea wrote:

> 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.

I think it's mostly a question of allowing uniform handling: Once you work on MSSA it is convenient if all your loads/stores are represented as memory accesses. Not having MemoryUses for invariant loads doesn't fundamentally prevent doing load CSE for them, but probably makes it a good bit more awkward. (It's something of a speculative concern though, as I don't think we really have an accepted approach on how MSSA should be introduced in GVN...)

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

I think I'd prefer this approach.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152743