[PATCH] D127383: Don't treat readnone call in presplit coroutine as not access memory

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

In D127383#3634475 <https://reviews.llvm.org/D127383#3634475>, @ChuanqiXu wrote:

> Given this is not dependent on D125291 <https://reviews.llvm.org/D125291>, do you think it is better to land this patch first? @nikic @nhaehnle @efriedma

Yes, this change can go in separately first.

In D127383#3634913 <https://reviews.llvm.org/D127383#3634913>, @ChuanqiXu wrote:

> Thanks! The original discussion is really long. But I am sure the direction is agreed by at least @jyknight @nhaehnle and @efriedma

Yes, this is the direction agreed upon: just checking "am I in a coroutine" in appropriate places, and pretending we didn't see the readnone/etc data -- with a potential long-term goal to incrementally teach passes to be smarter and understand the concept of "reads only thread-id", so they don't need to just give up entirely.



================
Comment at: llvm/docs/LangRef.rst:1943
+
+    Note that the readnone calls may access ``thread-id`` in a presplit coroutine.
 ``readonly``
----------------
nikic wrote:
> Drop "the"
They can do so always, not just in a presplit coroutine. And "thread-id" isn't a defined term anywhere. 

So I think this should probably say "Accessing the current thread's identity, e.g. getting the address of a thread-local variable is not considered a memory read."



================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:778-780
+  if (Call->hasFnAttr(Attribute::ReadNone) &&
+      Call->getFunction()->isPresplitCoroutine())
+    return FMRB_OnlyReadsMemory;
----------------
ChuanqiXu wrote:
> nikic wrote:
> > ChuanqiXu wrote:
> > > nikic wrote:
> > > > ChuanqiXu wrote:
> > > > > This early return is necessary otherwise it would fall to the combine operation at line 804, which would return FMRB_DoesNotAccessMemory.
> > > > You mean the getModRefBehavior call on the function below? I think it may be better to guard that call instead.
> > > Yeah, I mean the call at line 781. I feel it looks better/cleaner/clearer to put the check here. The logic is consistent with the above check (We can't do better.)
> > With your current implementation, won't you still run into a problem with a writeonly function, which will report as writeonly from function FMRB?
> Oh, I missed it. Thanks for pointing it out. I'm working on it.
I don't understand this change. ReadNone/WriteOnly being present on the call versus being present the function doesn't make a difference to the semantics vs presplit coroutines, unlike what's the case for operand bundles.

Hm...oh, ok...I see why you're doing this. Function::doesNotAccessMemory doesn't (can't) be modified to return false if we're in a coroutine, so we don't get the special-cased behavior there. That divergence is sufficiently confusing that I think we should probably not implement it that way.

Just looking at the calls to "doesNotAccessMemory", I wonder if we may be better off moving the query to the callers, anyways, instead of modifying Function/Call doesNotAccessMemory (and friends) themselves. AFAICT, it's not actually correct for all of the callers to get the new behavior.


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

https://reviews.llvm.org/D127383