[PATCH] D115302: GlobalsModRef should treat functions w/o nosync conservatively.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/GlobalsModRef.cpp:957-958
+                Call->hasFnAttr(Attribute::NoSync) ? ModRefInfo::NoModRef
+                : Call->onlyReadsMemory()          ? ModRefInfo::Ref
+                                                   : ModRefInfo::ModRef;
+            Known = unionModRef(
----------------
jdoerfert wrote:
> efriedma wrote:
> > arsenm wrote:
> > > tra wrote:
> > > > efriedma wrote:
> > > > > tra wrote:
> > > > > > I'm not quite sure whether this is completely correct.  
> > > > > > 
> > > > > > What if it's an intrinsic w/o `IntrNoSync` but with `IntrNoMem` property, which technically also indicates no side effects. 
> > > > > > 
> > > > > readnone and readonly both imply nosync; we treat synchronization as a read and write to memory.
> > > > > 
> > > > > In terms of intrinsics, we currently don't consistently add nosync attributes.  If we mark an intrinsic IntrNoMem, that should imply nosync, even without IntrNoSync. We shouldn't be trying to infer nosync from readnone/readonly in GlobalsAA.  We should probably hack IntrinsicEmitter::EmitAttributes to make that work.
> > > > > readnone and readonly both imply nosync; we treat synchronization as a read and write to memory.
> > > > 
> > > > > In terms of intrinsics, we currently don't consistently add nosync attributes. If we mark an intrinsic IntrNoMem, that should imply nosync, even without IntrNoSync. 
> > > > > We shouldn't be trying to infer nosync from readnone/readonly in GlobalsAA. We should probably hack IntrinsicEmitter::EmitAttributes to make that work.
> > > > 
> > > > So, IIUIC you're proposing to set `nosync` on intrinsics if either `IntrNoSync` or `IntrNoMem` properties are set. This part makes sense. I'll update the patch.
> > > > I think we should also infer `nosync` from IntrReadMem, IntrWriteMem, and  IntrArgMemOnly  as they all are documented as "and has no other side effects".
> > > > 
> > > > > We shouldn't be trying to infer nosync from readnone/readonly in GlobalsAA.
> > > > 
> > > > I assume you're talking about the `Call->onlyReadsMemory() ? Ref : ModRef` bit above. 
> > > > 
> > > > The idea is not to infer nosync, but rather to establish the lower bound on the valid result when `nosync` is absent based on what we do know about the function. 
> > > > It's equivalent of `ConservativeResult` in `getModRefInfoForArgument()` above. 
> > > > 
> > > > That said, it appears that there's a difference between intrinsic's `IntrReadMem` which is documented as  "It does not write to memory and has no other side effects." and  function's `readonly` attribute which does *not* imply "no other side effects". 
> > > > 
> > > > So,  for a regular function w/o `nosync` we may need to return ModRef, even if the function has `readonly` attribute.
> > > > 
> > > > 
> > > > readnone and readonly both imply nosync; we treat synchronization as a read and write to memory.
> > > This is not true. This is specifically why nosync is a separate attribute for non-memory communication
> > > > readnone and readonly both imply nosync; we treat synchronization as a read and write to memory.
> > > This is not true. This is specifically why nosync is a separate attribute for non-memory communication
> > 
> > The status quo is the following:
> > 
> > 1. AliasAnalysis and similar queries currently don't check for nosync on readonly/readnone calls; they assume such a call doesn't appear to modify memory.
> > 2. We don't infer readonly/readnone for functions which perform synchronization.
> > 3. We don't mark intrinsics which perform synchronization readonly/readnone.
> > 
> > In sum, LLVM behaves as if readonly/readnone implies nosync, and has behaved this way since before the nosync attribute was introduced in D62766.
> > 
> > We could potentially mess with this, I guess, but I don't see any pressing reason we'd want to.
> > In terms of intrinsics, we currently don't consistently add nosync attributes
> 
> This is not entirely true. Target independent intrinsics, AArch64, and NVVM all use DefaultIntrinsics which include nosync.
> Other targets will not update their td files as it becomes necessary.
> 
> 
> > So, IIUIC you're proposing to set nosync on intrinsics if either IntrNoSync or IntrNoMem properties are set. This part makes sense. I'll update the patch.
> 
> No, please don't. Nosync is by design not implied by readnone:
> LangRef: "Note that through convergent function calls non-memory communication, e.g., cross-lane operations, are possible and are also considered synchronization."
> The reasoning for the contrary come from a time before synchronization was a dedicated effect. A time where we needed memory for everything, incl. sync and termination.
> 
> > The status quo is the following:
> > ...
> 
> 3. Is certainly not true. From that, 2. is neither. Which makes 1. a wrong assumption.
> 
> Here is the nvptx and amdgcn intrinsic for shuffle: https://godbolt.org/z/TaWMT89G4
> One time it's readnone, one time inaccessible memory only, both will expose the GVN problem.
> 
> 
> > We could potentially mess with this, I guess, but I don't see any pressing reason we'd want to.
> 
> That ship has sailed a long time ago, basically as we introduced nosync we explicitly changed this. There are leftover assumptions like 3 around but that is a different story.
> 
> 
> > We could potentially mess with this, I guess, but I don't see any pressing reason we'd want to.
> 
> That ship has sailed a long time ago, basically as we introduced nosync we explicitly changed this. There are leftover assumptions like 3 around but that is a different story.

The introduction of nosync retroactively changed the meaning of readnone?  That doesn't seem right; the nosync patch didn't change LangRef, nor did it change any code involving readonly/readnone.  And we haven't subsequently adjusted the code querying readonly/readnone, or any alias analysis code, or clang.  A couple mismarked GPU intrinsics doesn't mean the rest of the world is broken...

Anyway, if you do want to change/clarify this, please propose a LangRef patch and post a message on Discourse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115302