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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115302