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

[Artem Belevich via Phabricator via llvm-commits]

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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115302