Don't treat readnone call in presplit coroutine as not access memory

[ChuanqiXu]

Summary

To solve the readnone problems in coroutines. See https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015 for details.

According to the discussion, we decide to fix the problem by inserting isPresplitCoroutine() checks in different passes instead of wrapping/unwrapping readnone attributes in CoroEarly/CoroCleanup passes. In this direction, we might not be able to cover every case at first. Let's take a "find and fix" strategy.

[ChuanqiXu]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

This early return is necessary otherwise it would fall to the combine operation at line 804, which would return FMRB_DoesNotAccessMemory.

[nikic]

llvm/include/llvm/IR/InstrTypes.h

1857

As it is now much more extensively used, we probably should convert the "coroutine.presplit" attribute into an enum attribute to make these queries less expensive.

[ChuanqiXu]

llvm/include/llvm/IR/InstrTypes.h

1857

Got it. I would convert "coroutine.presplit" into an enum attribute before landing this one.

[nhaehnle]

Thank you for doing this. It looks alright to me.

[ChuanqiXu]

@jyknight @eli.friedman ping~

[efriedma]

We probably want to put a note in LangRef noting that "readnone" doesn't encompass the thread id in coroutines.

llvm/include/llvm/IR/InstrTypes.h

1856

Probably we should describe this in more detail in the coroutine documentation, and put a pointer to that documentation in the code.

[ChuanqiXu]

Address comments:

• Add description in LangRef.rst and Coroutines.rst

[ChuanqiXu]

llvm/include/llvm/IR/InstrTypes.h

1856

I've added a description in Coroutines.rst but it is not more detailed than this. Do you mean to give more thoughts we've made in https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015? I feel it is too wordy for readers.

[ChuanqiXu]

Given this is not dependent on D125291, do you think it is better to land this patch first? @nikic @nhaehnle @efriedma

[nikic]

Compile-time impact is low / acceptable: http://llvm-compile-time-tracker.com/compare.php?from=40a4078e14c2c6c5e2d0a1776285aa7491e791b3&to=d753f388d52778fff19b5a6d82a5b9c4869a4273&stat=instructions

I'm fine with this change, but I didn't follow the original discussion.

llvm/docs/LangRef.rst

1953

Drop "the"

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

You mean the getModRefBehavior call on the function below? I think it may be better to guard that call instead.

[ChuanqiXu]

In D127383#3634889, @nikic wrote:

Compile-time impact is low / acceptable: http://llvm-compile-time-tracker.com/compare.php?from=40a4078e14c2c6c5e2d0a1776285aa7491e791b3&to=d753f388d52778fff19b5a6d82a5b9c4869a4273&stat=instructions

I'm fine with this change, but I didn't follow the original discussion.

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

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

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

[nikic]

llvm/lib/Analysis/GlobalsModRef.cpp

270 ↗

(On Diff #440857)

I've taken the liberty of deleting this whole function in https://github.com/llvm/llvm-project/commit/4a579abd9f95bf9fda920759aced3874d04c5b9e. This was unnecessary code duplication with BasicAA.

[nikic]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

With your current implementation, won't you still run into a problem with a writeonly function, which will report as writeonly from function FMRB?

[ChuanqiXu]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

Oh, I missed it. Thanks for pointing it out. I'm working on it.

llvm/lib/Analysis/GlobalsModRef.cpp

270 ↗

(On Diff #440857)

This might not be right. After the patch, when I call GlobalsAAResult::getModRefBehavior(CallBase*), it would call https://github.com/llvm/llvm-project/blob/519d7876cbee5a5d3cd40d41525cd45e44fb07a8/llvm/include/llvm/Analysis/AliasAnalysis.h#L1236-L1238 all the time, which is not correct.

[ChuanqiXu]

llvm/lib/Analysis/GlobalsModRef.cpp

270 ↗

(On Diff #440857)

I think we could improve the original implementation. I thought it would call BasicAAResult::getModRefBehavior(CallBase*) too.

[nikic]

llvm/lib/Analysis/GlobalsModRef.cpp

270 ↗

(On Diff #440857)

This might not be right. After the patch, when I call GlobalsAAResult::getModRefBehavior(CallBase*), it would call https://github.com/llvm/llvm-project/blob/519d7876cbee5a5d3cd40d41525cd45e44fb07a8/llvm/include/llvm/Analysis/AliasAnalysis.h#L1236-L1238 all the time, which is not correct.

This is fine. GlobalsAAResult methods are not intended to be called directly -- the public alias analysis API goes through an AAResults aggregate over multiple AA providers, which always includes BasicAA in a real pipeline. AA providers should not reimplement functionality provided by other AA providers, they are designed to compose via AAResults instead.

[ChuanqiXu]

Address comments:

• Handle write only cases.

[ChuanqiXu]

llvm/lib/Analysis/GlobalsModRef.cpp

270 ↗

(On Diff #440857)

Thanks for clarifying!

[jyknight]

In D127383#3634475, @ChuanqiXu wrote:

Given this is not dependent on 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, @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.

llvm/docs/LangRef.rst

1953

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

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

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.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

755–757

I think it is better to modify CallBase::doesNotAccessMemory instead of modify the call sites.

For the perspective of semantics, we've changed the semantics for readnone attribute. It should be naturally correct to modify Call:: doesNotAccessMemory to show the change.

For the perspective of engineering, I think the current method is better too. What I imaged is a new developer who wants to develop a new optimization pass. The implementation calls CallBase:: doesNotAccessMemory at several places. But the implementation may not be right since he forgets to add the in presplit coroutine checks. What I want to say is that it requires the developers more if we choose to add the check at call sites. But we could avoid it.

[ChuanqiXu]

@jyknight gentle ping~

[nikic]

Do we also need to upgrade argmemonly to inaccessibleorargmemonly? Assuming that the thread ID counts as inaccessible memory.

llvm/include/llvm/IR/InstrTypes.h

1854

couldn't -> can't, wouldn't -> won't

1871

Due to -> Because

llvm/test/Transforms/Coroutines/coro-readnone-01.ll

6

It would be better to consistently use ptr in the test and drop the -opaque-pointers flag. Currently it mixed ptr and i8*...

[nhaehnle]

In D127383#3650988, @nikic wrote:

Do we also need to upgrade argmemonly to inaccessibleorargmemonly? Assuming that the thread ID counts as inaccessible memory.

Nit: I wouldn't say thread ID counts as inaccessible memory, rather it's something that is apart entirely (given the change for readnone).

I think it would be consistent to say that an argmemonly function is allowed to read the thread ID. That does suggest that analogous changes for onlyAccessesArgMemory are needed, but I haven't thought about it very carefully.

[nikic]

In D127383#3651637, @nhaehnle wrote:

In D127383#3650988, @nikic wrote:

Do we also need to upgrade argmemonly to inaccessibleorargmemonly? Assuming that the thread ID counts as inaccessible memory.

Nit: I wouldn't say thread ID counts as inaccessible memory, rather it's something that is apart entirely (given the change for readnone).

I think it would be consistent to say that an argmemonly function is allowed to read the thread ID. That does suggest that analogous changes for onlyAccessesArgMemory are needed, but I haven't thought about it very carefully.

Yeah, you're right, it's not inaccessible memory under this model. I guess onlyAccessesInaccessibleMemory and onlyAccessesInaccessibleMemOrArgMem would have to be changed as well. Kinda unfortunate in that things like llvm.assume go from modelling a control dependence to doing an arbitrary read.

[nhaehnle]

Yeah, you're right, it's not inaccessible memory under this model. I guess onlyAccessesInaccessibleMemory and onlyAccessesInaccessibleMemOrArgMem would have to be changed as well. Kinda unfortunate in that things like llvm.assume go from modelling a control dependence to doing an arbitrary read.

True, but at least it's only pre-split. In the Discourse thread we did talk about adding a noreadthreadid attribute, though I think the preliminary conclusion was to wait and see if it'd be actually worth doing the change.

[rjmccall]

Right. In order to get back to having this optimization power even within unsplit coroutines, we have to start modeling the effect of being dependent on the identity of current thread. That effect includes reading the current thread ID, taking the address of thread-local memory, etc. And the right way to do that is to add a nothreadid attribute (or however we decide to spell it) that calls can affirmatively say they have. Once we have that in the IR, we can also talk about having a language feature that lowers to it (e.g. an attribute stronger than __attribute__((const))).

[ChuanqiXu]

Address comments: Handle onlyAccessesInaccessibleMemory and onlyAccessesInaccessibleMemOrArgMem.

[ChuanqiXu]

Yeah, it looks better to add things like noreadthreadid in the future.

[nikic]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

779

Do we lose anything substantial with just the Call->getFunction()->isPresplitCoroutine() condition?

Alternatively, I would implement this as a fixup afterwards that looks something like this:

if (Call->getFunction()->isPreSplitCoroutine())
  Min = FunctionModRefBehavior(Min | FMRB_OnlyReadsMemory);

[ChuanqiXu]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

779

For,

define void @f() presplitcoroutine  {
      entry:
        %ArgMemOnlyCall = call i32 @argmemonly_func()
        ret void
      }

      declare i32 @argmemonly_func() argmemonly

The current implementation would get FMRB_OnlyAccessesArgumentPointees for ArgMemOnlyCall. But the suggested change would get FMRB_UnknownModRefBehavior. I am OK with the suggested change if you feel like the benefit is not worth for the cost. I know compilation time is an important feature of Clang/LLVM.

[nikic]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

779

But isn't FMRB_OnlyAccessesArgumentPointees incorrect, because the thread ID is not an argument, so it accesses non-argument memory?

[ChuanqiXu]

Handle argmemonly.

[ChuanqiXu]

llvm/lib/Analysis/BasicAliasAnalysis.cpp

779

Oh, I missed that case mentioned by @nhaehnle. Then the current style would return FMRB_UnknownModRefBehavior for ReadOnlyCall but the previous style would return FMRB_OnlyReadsMemory.

define void @f() presplitcoroutine  {
      entry:
        %ReadOnlyCall = call i32 @readonly_func()
        ret void
}

declare i32 @ readonly_func() readonly

But I feel like the benefit is not worthy. So I follow your suggestion in this revision.

[nikic]

LGTM, but please wait a day in case there are more comments.

llvm/lib/Analysis/BasicAliasAnalysis.cpp

771

writelonly -> writeonly

[ChuanqiXu]

In D127383#3659673, @nikic wrote:

LGTM, but please wait a day in case there are more comments.

Thanks for reviewing!

[ChuanqiXu]

I met 2 crashes after landing the patch. From my point of view, the problems of the 2 crashes lives somewhere else. I sent 2 draft fixes D130153 and D130155 to fix these 2 crashes. But I am not sure if these 2 fixes are good and I reverted this one to avoid disturbing any one else.

[fhahn]

The follow-up changes required by this patch (D130155, D130153) seem to highlight some gaps with the new modeling . As a side-effect we now consider intrinsics we know *won't* read the thread id as may-read, even if they are marked readonly, which breaks assumptions in a couple of places. There probably will be more. It seems like we should at least have a way to mark functions we know don't access thread ids, rather than updating code to account for the fact that read none intrinsics now are considered may-read in some circumstances.

I have not been through all the previous discussion, but from the langref changes the new behavior of doesNotAccessMemory & co is not very clear to me. The langref change specifically calls out reading the id of the current thread being not considered reading memory. But doesNotAccessMemory seems to consider it accessing memory in Coro-split functions? Shouldn't the LangRef's definition at least clearly call out the interaction with the presplitcoroutine attribute?

In general, reasoning about the interaction between various memory attributes is already quite tricky and making their behavior dependent on a different attribute seems to make it even more difficult.

[efriedma]

It seems like we should at least have a way to mark functions we know don't access thread ids, rather than updating code to account for the fact that read none intrinsics now are considered may-read in some circumstances.

The overall plan involves adding a "nothreadid" attribute (name subject to bikeshedding) at some point. But it was left out of the initial patch, since it's not critical for correctness.

I have not been through all the previous discussion, but from the langref changes the new behavior of doesNotAccessMemory & co is not very clear to me. The langref change specifically calls out reading the id of the current thread being not considered reading memory. But doesNotAccessMemory seems to consider it accessing memory in Coro-split functions? Shouldn't the LangRef's definition at least clearly call out the interaction with the presplitcoroutine attribute?

https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/48 is a summary of the overall design here. I'd suggest reading the rest of the discussion if you want more context... it's hard to summarize the discussion, but we concluded this was the least disruptive solution.

It might be worth explicitly calling out the interaction with coroutines more explicitly in LangRef, sure.

[ChuanqiXu]

In D127383#3676652, @fhahn wrote:

The follow-up changes required by this patch (D130155, D130153) seem to highlight some gaps with the new modeling . As a side-effect we now consider intrinsics we know *won't* read the thread id as may-read, even if they are marked readonly, which breaks assumptions in a couple of places. There probably will be more.

(Besides what Eli has said about the modeling)

It surprised me too that the patch would cause crash. I thought the only effect of the patch is to block some optimizations in presplit coroutines and this is the known cost. And for the assumptions, we would meet crash once we break it. So it looks like the range of the assumptions could be tested. And I've tested our internal workloads and folly (The largest open sourced user of coroutines I know) and it looks fine. So I think the places would not be too much.

[jdoerfert]

FWIW, https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/55

[fhahn]

In D127383#3677020, @efriedma wrote:

https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/48 is a summary of the overall design here. I'd suggest reading the rest of the discussion if you want more context... it's hard to summarize the discussion, but we concluded this was the least disruptive solution.

It might be worth explicitly calling out the interaction with coroutines more explicitly in LangRef, sure.

Thanks Eli, that's very helpful message to read! IIUC the plan is to an extra attribute to allow the distinction between read none & not reading thread id, which should be used for all/most current intrinsics. This should avoid the need for workarounds like D130155 & D130153 by better modeling semantics. IMO this would be practical reasons to adding the attribute up front.

[ChuanqiXu]

In D127383#3699441, @fhahn wrote:

In D127383#3677020, @efriedma wrote:

https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/48 is a summary of the overall design here. I'd suggest reading the rest of the discussion if you want more context... it's hard to summarize the discussion, but we concluded this was the least disruptive solution.

It might be worth explicitly calling out the interaction with coroutines more explicitly in LangRef, sure.

Thanks Eli, that's very helpful message to read! IIUC the plan is to an extra attribute to allow the distinction between read none & not reading thread id, which should be used for all/most current intrinsics. This should avoid the need for workarounds like D130155 & D130153 by better modeling semantics. IMO this would be practical reasons to adding the attribute up front.

Do you suggest to add the noread_threadid attribute first? So that we could emit noread_threadid for intrinsics like memset or dbg.declare so that we could skip some workarounds like D130155 & D130153. Do I understand right? IIUC, I'm OK since 15.x is already branched and we have enough time.

[ChuanqiXu]

We prefer: https://reviews.llvm.org/D135550

llvm/docs/Coroutines.rst

 
 #. Cannot handle coroutines with `inalloca` parameters (used in x86 on Windows).
 
 #. Alignment is ignored by coro.begin and coro.free intrinsics.
 
 #. Make required changes to make sure that coroutine optimizations work with
    LTO.
 
+#. A readnone/writeonly call may access memory in a presplit coroutine. Since
+thread-id was assumed to be a constant in a function historically. But it is
+not true for coroutines.
+
 #. More tests, more tests, more tests

llvm/docs/LangRef.rst

``inaccessiblememonly``
     function. This is a weaker form of ``readnone``. If the function reads
     or writes other memory, the behavior is undefined.
 
     For clarity, note that such functions are allowed to return new memory
     which is ``noalias`` with respect to memory already accessible from
     the module.  That is, a function can be both ``inaccessiblememonly`` and
     have a ``noalias`` return which introduces a new, potentially initialized,
     allocation.
+
+    Note that accessing the current thread's identity, e.g. getting the address
+    of a thread-local variable is not considered a memory read.
 ``inaccessiblemem_or_argmemonly``
     This attribute indicates that the function may only access memory that is
     either not accessible by the module being compiled, or is pointed to
     by its pointer arguments. This is a weaker form of  ``argmemonly``. If the
     function reads or writes other memory, the behavior is undefined.
+
+    Note that accessing the current thread's identity, e.g. getting the address
+    of a thread-local variable is not considered a memory read.
 ``inlinehint``
     This attribute indicates that the source code contained a hint that
     inlining this function is desirable (such as the "inline" keyword in
     C/C++). It is just a hint; it imposes no requirements on the
     inliner.
 ``jumptable``
     This attribute indicates that the function should be added to a
     jump-instruction table at code-generation time, and that all address-taken
``readnone``
     On an argument, this attribute indicates that the function does not
     dereference that pointer argument, even though it may read or write the
     memory that the pointer points to if accessed through other pointers.
 
     If a readnone function reads or writes memory visible outside the function,
     or has other side-effects, the behavior is undefined. If a
     function reads from or writes to a readnone pointer argument, the behavior
     is undefined.
+
+    Note that accessing the current thread's identity, e.g. getting the address
+    of a thread-local variable is not considered a memory read.
 ``readonly``
     On a function, this attribute indicates that the function does not write
     through any pointer arguments (including ``byval`` arguments) or otherwise
     modify any state (e.g. memory, control registers, etc) visible outside the
     ``readonly`` function. It may dereference pointer arguments and read
     state that may be set in the caller. A readonly function always
     returns the same value (or unwinds an exception identically) when
     called with the same set of arguments and global state.  This means while it
``writeonly``
 
     On an argument, this attribute indicates that the function may write to but
     does not read through this pointer argument (even though it may read from
     the memory that the pointer points to).
 
     If a writeonly function reads memory visible outside the function or has
     other side-effects, the behavior is undefined. If a function reads
     from a writeonly pointer argument, the behavior is undefined.
+
+    Note that accessing the current thread's identity, e.g. getting the address
+    of a thread-local variable is not considered a memory read.
 ``argmemonly``
     This attribute indicates that the only memory accesses inside function are
     loads and stores from objects pointed to by its pointer-typed arguments,
     with arbitrary offsets. Or in other words, all memory operations in the
     function can refer to memory only using pointers based on its function
     arguments.
 
     Note that ``argmemonly`` can be used together with ``readonly`` attribute

llvm/include/llvm/IR/InstrTypes.h

public:
 
   /// Determine if the call requires strict floating point semantics.
   bool isStrictFP() const { return hasFnAttr(Attribute::StrictFP); }
 
   /// Return true if the call should not be inlined.
   bool isNoInline() const { return hasFnAttr(Attribute::NoInline); }
   void setIsNoInline() { addFnAttr(Attribute::NoInline); }
   /// Determine if the call does not access memory.
-  bool doesNotAccessMemory() const { return hasFnAttr(Attribute::ReadNone); }
+  bool doesNotAccessMemory() const {
+    return hasFnAttr(Attribute::ReadNone) &&
+           // If the call lives in presplit coroutine, we can't assume the
+           // call won't access memory even if it has readnone attribute.
+           // Since readnone could be used for thread identification and
+           // coroutines might resume in different threads.
+           (!getFunction() || !getFunction()->isPresplitCoroutine());
+  }
   void setDoesNotAccessMemory() { addFnAttr(Attribute::ReadNone); }
 
   /// Determine if the call does not access or only reads memory.
   bool onlyReadsMemory() const {
     return hasImpliedFnAttr(Attribute::ReadOnly);
   }
 
   void setOnlyReadsMemory() { addFnAttr(Attribute::ReadOnly); }
 
   /// Determine if the call does not access or only writes memory.
   bool onlyWritesMemory() const {
-    return hasImpliedFnAttr(Attribute::WriteOnly);
+    return hasImpliedFnAttr(Attribute::WriteOnly) &&
+           // See the comments in doesNotAccessMemory. Because readnone implies
+           // writeonly.
+           (!getFunction() || !getFunction()->isPresplitCoroutine());
   }
   void setOnlyWritesMemory() { addFnAttr(Attribute::WriteOnly); }
 
   /// Determine if the call can access memmory only using pointers based
   /// on its arguments.
   bool onlyAccessesArgMemory() const {
-    return hasFnAttr(Attribute::ArgMemOnly);
+    return hasFnAttr(Attribute::ArgMemOnly) &&
+        // Thread ID don't count as inaccessible memory. And thread ID don't
+        // count as constant in presplit coroutine.
+        (!getFunction() || !getFunction()->isPresplitCoroutine());;
   }
   void setOnlyAccessesArgMemory() { addFnAttr(Attribute::ArgMemOnly); }
 
   /// Determine if the function may only access memory that is
   /// inaccessible from the IR.
   bool onlyAccessesInaccessibleMemory() const {
-    return hasFnAttr(Attribute::InaccessibleMemOnly);
+    return hasFnAttr(Attribute::InaccessibleMemOnly) &&
+        // Thread ID don't count as inaccessible memory. And thread ID don't
+        // count as constant in presplit coroutine.
+        (!getFunction() || !getFunction()->isPresplitCoroutine());
   }
   void setOnlyAccessesInaccessibleMemory() {
     addFnAttr(Attribute::InaccessibleMemOnly);
   }
 
   /// Determine if the function may only access memory that is
   /// either inaccessible from the IR or pointed to by its arguments.
   bool onlyAccessesInaccessibleMemOrArgMem() const {
-    return hasFnAttr(Attribute::InaccessibleMemOrArgMemOnly);
+    return hasFnAttr(Attribute::InaccessibleMemOrArgMemOnly) &&
+        // Thread ID don't count as inaccessible memory. And thread ID don't
+        // count as constant in presplit coroutine.
+        (!getFunction() || !getFunction()->isPresplitCoroutine());
   }
   void setOnlyAccessesInaccessibleMemOrArgMem() {
     addFnAttr(Attribute::InaccessibleMemOrArgMemOnly);
   }
   /// Determine if the call cannot return.
   bool doesNotReturn() const { return hasFnAttr(Attribute::NoReturn); }
   void setDoesNotReturn() { addFnAttr(Attribute::NoReturn); }
 

llvm/lib/Analysis/BasicAliasAnalysis.cpp

FunctionModRefBehavior BasicAAResult::getModRefBehavior(const CallBase *Call) {
   if (Call->doesNotAccessMemory())
     // Can't do better than this.
     return FMRB_DoesNotAccessMemory;
 
   FunctionModRefBehavior Min = FMRB_UnknownModRefBehavior;
 
   // If the callsite knows it only reads memory, don't return worse
   // than that.
   if (Call->onlyReadsMemory())
     Min = FMRB_OnlyReadsMemory;
   else if (Call->onlyWritesMemory())
     Min = FMRB_OnlyWritesMemory;
 
   if (Call->onlyAccessesArgMemory())
     Min = FunctionModRefBehavior(Min & FMRB_OnlyAccessesArgumentPointees);
   else if (Call->onlyAccessesInaccessibleMemory())
     Min = FunctionModRefBehavior(Min & FMRB_OnlyAccessesInaccessibleMem);
   else if (Call->onlyAccessesInaccessibleMemOrArgMem())
     Min = FunctionModRefBehavior(Min & FMRB_OnlyAccessesInaccessibleOrArgMem);
 
   // If the call has operand bundles then aliasing attributes from the function
   // it calls do not directly apply to the call.  This can be made more precise
   // in the future.
-  if (!Call->hasOperandBundles())
+  //
+  // If the call lives in a presplit coroutine, the readnone, writeonly,
+  // inaccessiblememonly and inaccessiblemem_or_argmemonly attribute from the
+  // function might not directly apply to the call.
+  if (!Call->hasOperandBundles() && !Call->getFunction()->isPresplitCoroutine())
     if (const Function *F = Call->getCalledFunction())
       Min =
           FunctionModRefBehavior(Min & getBestAAResults().getModRefBehavior(F));
 
   return Min;
 }
 
 /// Returns the behavior when calling the given function. For use when the call
 /// site is not known.
 FunctionModRefBehavior BasicAAResult::getModRefBehavior(const Function *F) {
   // If the function declares it doesn't access memory, we can't do better.
   if (F->doesNotAccessMemory())
     return FMRB_DoesNotAccessMemory;

llvm/test/Transforms/Coroutines/coro-readnone-01.ll

+; Tests that the readnone function which cross suspend points wouldn't be misoptimized.
+; RUN: opt < %s -S -passes='default<O3>' | FileCheck %s --check-prefixes=CHECK,CHECK_SPLITTED
+; RUN: opt < %s -S -passes='early-cse' | FileCheck %s --check-prefixes=CHECK,CHECK_UNSPLITTED
+; RUN: opt < %s -S -passes='gvn' | FileCheck %s --check-prefixes=CHECK,CHECK_UNSPLITTED
+; RUN: opt < %s -S -passes='newgvn' | FileCheck %s --check-prefixes=CHECK,CHECK_UNSPLITTED
+
+define ptr @f() presplitcoroutine {
+entry:
+  %id = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
+  %size = call i32 @llvm.coro.size.i32()
+  %alloc = call ptr @malloc(i32 %size)
+  %hdl = call ptr @llvm.coro.begin(token %id, ptr %alloc)
+  %j = call i32 @readnone_func() readnone
+  %sus_result = call i8 @llvm.coro.suspend(token none, i1 false)
+  switch i8 %sus_result, label %suspend [i8 0, label %resume
+                                         i8 1, label %cleanup]
+resume:
+  %i = call i32 @readnone_func() readnone
+  %cmp = icmp eq i32 %i, %j
+  br i1 %cmp, label %same, label %diff
+
+same:
+  call void @print_same()
+  br label %cleanup
+
+diff:
+  call void @print_diff()
+  br label %cleanup
+
+cleanup:
+  %mem = call ptr @llvm.coro.free(token %id, ptr %hdl)
+  call void @free(ptr %mem)
+  br label %suspend
+
+suspend:
+  call i1 @llvm.coro.end(ptr %hdl, i1 0)
+  ret ptr %hdl
+}
+
+; Tests that normal functions wouldn't be affected.
+define i1 @normal_function() {
+entry:
+  %i = call i32 @readnone_func() readnone
+  %j = call i32 @readnone_func() readnone
+  %cmp = icmp eq i32 %i, %j
+  br i1 %cmp, label %same, label %diff
+
+same:
+  call void @print_same()
+  ret i1 true
+
+diff:
+  call void @print_diff()
+  ret i1 false
+}
+
+; CHECK_SPLITTED-LABEL: normal_function(
+; CHECK_SPLITTED-NEXT: entry
+; CHECK_SPLITTED-NEXT:   call i32 @readnone_func()
+; CHECK_SPLITTED-NEXT:   call void @print_same()
+; CHECK_SPLITTED-NEXT:   ret i1 true
+;
+; CHECK_SPLITTED-LABEL: f.resume(
+; CHECK_UNSPLITTED-LABEL: @f(
+; CHECK: br i1 %cmp, label %same, label %diff
+; CHECK-EMPTY:
+; CHECK-NEXT: same:
+; CHECK-NEXT:   call void @print_same()
+; CHECK-NEXT:   br label
+; CHECK-EMPTY:
+; CHECK-NEXT: diff:
+; CHECK-NEXT:   call void @print_diff()
+; CHECK-NEXT:   br label
+
+declare i32 @readnone_func() readnone
+
+declare void @print_same()
+declare void @print_diff()
+declare ptr @llvm.coro.free(token, ptr)
+declare i32 @llvm.coro.size.i32()
+declare i8  @llvm.coro.suspend(token, i1)
+
+declare token @llvm.coro.id(i32, ptr, ptr, ptr)
+declare i1 @llvm.coro.alloc(token)
+declare ptr @llvm.coro.begin(token, ptr)
+declare i1 @llvm.coro.end(ptr, i1)
+
+declare noalias ptr @malloc(i32)
+declare void @free(ptr)

llvm/test/Transforms/Coroutines/coro-readnone-02.ll

+; Tests that the readnone function which don't cross suspend points could be optimized expectly after split.
+;
+; RUN: opt < %s -S -passes='default<O3>' | FileCheck %s --check-prefixes=CHECK_SPLITTED
+; RUN: opt < %s -S -passes='coro-split,early-cse,simplifycfg' | FileCheck %s --check-prefixes=CHECK_SPLITTED
+; RUN: opt < %s -S -passes='coro-split,gvn,simplifycfg' | FileCheck %s --check-prefixes=CHECK_SPLITTED
+; RUN: opt < %s -S -passes='coro-split,newgvn,simplifycfg' | FileCheck %s --check-prefixes=CHECK_SPLITTED
+; RUN: opt < %s -S -passes='early-cse' | FileCheck %s --check-prefixes=CHECK_UNSPLITTED
+; RUN: opt < %s -S -passes='gvn' | FileCheck %s --check-prefixes=CHECK_UNSPLITTED
+; RUN: opt < %s -S -passes='newgvn' | FileCheck %s --check-prefixes=CHECK_UNSPLITTED
+
+define ptr @f() presplitcoroutine {
+entry:
+  %id = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
+  %size = call i32 @llvm.coro.size.i32()
+  %alloc = call ptr @malloc(i32 %size)
+  %hdl = call ptr @llvm.coro.begin(token %id, ptr %alloc)
+  %sus_result = call i8 @llvm.coro.suspend(token none, i1 false)
+  switch i8 %sus_result, label %suspend [i8 0, label %resume
+                                         i8 1, label %cleanup]
+resume:
+  %i = call i32 @readnone_func() readnone
+  ; noop call to break optimization to combine two consecutive readonly calls.
+  call void @nop()
+  %j = call i32 @readnone_func() readnone
+  %cmp = icmp eq i32 %i, %j
+  br i1 %cmp, label %same, label %diff
+
+same:
+  call void @print_same()
+  br label %cleanup
+
+diff:
+  call void @print_diff()
+  br label %cleanup
+
+cleanup:
+  %mem = call ptr @llvm.coro.free(token %id, ptr %hdl)
+  call void @free(ptr %mem)
+  br label %suspend
+
+suspend:
+  call i1 @llvm.coro.end(ptr %hdl, i1 0)
+  ret ptr %hdl
+}
+
+;
+; CHECK_SPLITTED-LABEL: f.resume(
+; CHECK_SPLITTED-NEXT:  :
+; CHECK_SPLITTED-NEXT:    call i32 @readnone_func() #[[ATTR_NUM:[0-9]+]]
+; CHECK_SPLITTED-NEXT:    call void @nop()
+; CHECK_SPLITTED-NEXT:    call void @print_same()
+;
+; CHECK_SPLITTED: attributes #[[ATTR_NUM]] = { readnone }
+;
+; CHECK_UNSPLITTED-LABEL: @f(
+; CHECK_UNSPLITTED: br i1 %cmp, label %same, label %diff
+; CHECK_UNSPLITTED-EMPTY:
+; CHECK_UNSPLITTED-NEXT: same:
+; CHECK_UNSPLITTED-NEXT:   call void @print_same()
+; CHECK_UNSPLITTED-NEXT:   br label
+; CHECK_UNSPLITTED-EMPTY:
+; CHECK_UNSPLITTED-NEXT: diff:
+; CHECK_UNSPLITTED-NEXT:   call void @print_diff()
+; CHECK_UNSPLITTED-NEXT:   br label
+
+declare i32 @readnone_func() readnone
+declare void @nop()
+
+declare void @print_same()
+declare void @print_diff()
+declare ptr @llvm.coro.free(token, ptr)
+declare i32 @llvm.coro.size.i32()
+declare i8  @llvm.coro.suspend(token, i1)
+
+declare token @llvm.coro.id(i32, ptr, ptr, ptr)
+declare i1 @llvm.coro.alloc(token)
+declare ptr @llvm.coro.begin(token, ptr)
+declare i1 @llvm.coro.end(ptr, i1)
+
+declare noalias ptr @malloc(i32)
+declare void @free(ptr)

llvm/unittests/Analysis/AliasAnalysisTest.cpp

TEST_F(AliasAnalysisTest, PartialAliasOffsetSign) {
   auto AR = AA.alias(Loc1, Loc2);
   EXPECT_EQ(AR, AliasResult::PartialAlias);
   EXPECT_EQ(1, AR.getOffset());
 
   AR = AA.alias(Loc2, Loc1);
   EXPECT_EQ(AR, AliasResult::PartialAlias);
   EXPECT_EQ(-1, AR.getOffset());
 }
+
+TEST_F(AliasAnalysisTest, AAInCoroutines) {
+  LLVMContext C;
+  SMDiagnostic Err;
+  std::unique_ptr<Module> M = parseAssemblyString(R"(
+      define void @f() presplitcoroutine  {
+      entry:
+        %ReadNoneCall = call i32 @readnone_func() readnone
+        %WriteOnlyCall = call i32 @writeonly_func() writeonly
+        %ArgMemOnlyCall = call i32 @argmemonly_func() argmemonly
+        %OnlyAccessesInaccessibleMemoryCall = call i32 @only_accesses_inaccessible_memory_call() inaccessiblememonly
+        %OnlyAccessesInaccessibleMemOrArgMemCall = call i32 @only_accesses_inaccessible_memory_or_argmemonly_call() inaccessiblemem_or_argmemonly
+        ret void
+      }
+
+      declare i32 @readnone_func() readnone
+      declare i32 @writeonly_func() writeonly
+      declare i32 @argmemonly_func() argmemonly
+      declare i32 @only_accesses_inaccessible_memory_call() inaccessiblememonly
+      declare i32 @only_accesses_inaccessible_memory_or_argmemonly_call() inaccessiblemem_or_argmemonly
+    )",
+                                                  Err, C);
+
+  ASSERT_TRUE(M);
+  Function *F = M->getFunction("f");
+  CallInst *ReadNoneCall =
+      cast<CallInst>(getInstructionByName(*F, "ReadNoneCall"));
+
+  auto &AA = getAAResults(*F);
+  EXPECT_FALSE(AA.doesNotAccessMemory(ReadNoneCall));
+  EXPECT_TRUE(AA.onlyReadsMemory(ReadNoneCall));
+
+  EXPECT_EQ(FMRB_OnlyReadsMemory, AA.getModRefBehavior(ReadNoneCall));
+
+  CallInst *WriteOnlyCall =
+      cast<CallInst>(getInstructionByName(*F, "WriteOnlyCall"));
+  EXPECT_EQ(FMRB_UnknownModRefBehavior, AA.getModRefBehavior(WriteOnlyCall));
+
+  CallInst *ArgMemOnlyCall =
+      cast<CallInst>(getInstructionByName(*F, "ArgMemOnlyCall"));
+  EXPECT_EQ(FMRB_UnknownModRefBehavior,
+            AA.getModRefBehavior(ArgMemOnlyCall));
+
+  CallInst *OnlyAccessesInaccessibleMemoryCall =
+      cast<CallInst>(getInstructionByName(*F, "OnlyAccessesInaccessibleMemoryCall"));
+  EXPECT_EQ(FMRB_UnknownModRefBehavior,
+            AA.getModRefBehavior(OnlyAccessesInaccessibleMemoryCall));
+
+  CallInst *OnlyAccessesInaccessibleMemOrArgMemCall =
+      cast<CallInst>(getInstructionByName(*F, "OnlyAccessesInaccessibleMemOrArgMemCall"));
+  EXPECT_EQ(FMRB_UnknownModRefBehavior,
+            AA.getModRefBehavior(OnlyAccessesInaccessibleMemOrArgMemCall));
+}
+
 class AAPassInfraTest : public testing::Test {
 protected:
   LLVMContext C;
   SMDiagnostic Err;
   std::unique_ptr<Module> M;
 
 public:
   AAPassInfraTest()

llvm/unittests/IR/InstructionsTest.cpp

 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/FPEnv.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/NoFolder.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm-c/Core.h"
 #include "gmock/gmock-matchers.h"
TEST(InstructionsTest, AllocaInst) {
   EXPECT_FALSE(C.getAllocationSizeInBits(DL));
   EXPECT_EQ(D.getAllocationSizeInBits(DL), TypeSize::getFixed(512));
   EXPECT_EQ(E.getAllocationSizeInBits(DL), TypeSize::getScalable(512));
   EXPECT_EQ(F.getAllocationSizeInBits(DL), TypeSize::getFixed(32));
   EXPECT_EQ(G.getAllocationSizeInBits(DL), TypeSize::getFixed(768));
   EXPECT_EQ(H.getAllocationSizeInBits(DL), TypeSize::getFixed(160));
 }
 
+static Instruction *getInstructionByName(Function &F, StringRef Name) {
+  for (auto &I : instructions(F))
+    if (I.getName() == Name)
+      return &I;
+  llvm_unreachable("Expected to find instruction!");
+}
+
+TEST(InstructionsTest, CallInstInPresplitCoroutine) {
+  LLVMContext Ctx;
+  std::unique_ptr<Module> M = parseIR(Ctx, R"(
+      define void @f() presplitcoroutine  {
+      entry:
+        %ReadNoneCall = call i32 @readnone_func() readnone
+        %WriteOnlyCall = call i32 @writeonly_func() writeonly
+        %ArgMemOnlyCall = call i32 @argmemonly_func() argmemonly
+        %OnlyAccessesInaccessibleMemoryCall = call i32 @only_accesses_inaccessible_memory_call() inaccessiblememonly
+        %OnlyAccessesInaccessibleMemOrArgMemCall = call i32 @only_accesses_inaccessible_memory_or_argmemonly_call() inaccessiblemem_or_argmemonly
+        ret void
+      }
+
+      declare i32 @readnone_func() readnone
+      declare i32 @writeonly_func() writeonly
+      declare i32 @argmemonly_func() argmemonly
+      declare i32 @only_accesses_inaccessible_memory_call() inaccessiblememonly
+      declare i32 @only_accesses_inaccessible_memory_or_argmemonly_call() inaccessiblemem_or_argmemonly
+    )");
+
+  ASSERT_TRUE(M);
+  Function *F = M->getFunction("f");
+  CallInst *ReadNoneCall =
+      cast<CallInst>(getInstructionByName(*F, "ReadNoneCall"));
+  CallInst *WriteOnlyCall =
+      cast<CallInst>(getInstructionByName(*F, "WriteOnlyCall"));
+  CallInst *OnlyAccessesInaccessibleMemoryCall =
+      cast<CallInst>(getInstructionByName(*F, "OnlyAccessesInaccessibleMemoryCall"));
+  CallInst *OnlyAccessesInaccessibleMemOrArgMemCall =
+      cast<CallInst>(getInstructionByName(*F, "OnlyAccessesInaccessibleMemOrArgMemCall"));
+   CallInst *ArgMemOnlyCall =
+      cast<CallInst>(getInstructionByName(*F, "ArgMemOnlyCall"));
+
+  EXPECT_FALSE(ReadNoneCall->doesNotAccessMemory());
+  EXPECT_FALSE(ReadNoneCall->onlyWritesMemory());
+  EXPECT_TRUE(ReadNoneCall->onlyReadsMemory());
+
+  EXPECT_FALSE(WriteOnlyCall->onlyWritesMemory());
+
+  EXPECT_FALSE(OnlyAccessesInaccessibleMemoryCall->onlyAccessesInaccessibleMemory());
+
+  EXPECT_FALSE(OnlyAccessesInaccessibleMemOrArgMemCall->onlyAccessesInaccessibleMemOrArgMem());
+
+  EXPECT_FALSE(ArgMemOnlyCall->onlyAccessesArgMemory());
+}
+
 } // end anonymous namespace
 } // end namespace llvm