In D95186#2514080, @wenlei wrote:

The same check is done in InlineCost: https://github.com/llvm/llvm-project/blob/8b0bd54d0ec968df28ccc58bbb537a7b7c074ef2/llvm/lib/Analysis/InlineCost.cpp#L2537-L2552

The check in InlineCost is on Caller, so it's not strictly redundant.

In D94834#2504128, @ChuanqiXu wrote:

I am a little unclear about this problem. From my point of view, it seems like that there is a Coroutine C elided in a normal function F. Then in the Coroutine Body of C, it would try to switch to itself by symmetric transfer. However, the Coroutine frame of C now is a stack variable. Then the tail call would pass the address of the stack variable whose lifetime has ended, so here is the corruption. Did I understand the situation?

address comments

Thank you!

Thanks. I agree your fix is correct.
Some small suggestions commented

Thank you for working on this! LGTM. I will let others who are more familiar with SCC to accept it.
A few nits commented inline.

rebase

Update test. A few things: 1) we don't need and shouldn't add --coro-early. Both functions are already marked with coroutine.presplit=1, ready for CoroSplit. coro-early will actually mess it up. 2) It turns out we need the meta data and dbg info since SampleProfileLoader depends on it. 3) We don't need all the attributes. Removed the unused ones.

In D92662#2446777, @jyknight wrote:

I don't think we should change the meaning of __attribute__((const)) to exclude depending on thread-id.

However, if we do want to do so, and call the existing uses of __attribute__((const)) in glibc invalid, we need to special case many more functions. Looking through it a little, I see __errno_location, __rpc_thread_variables, __ctype_b_loc, __ctype_tolower_loc, __ctype_toupper_loc, __libc_tsd_address...and I gave up looking after that.

In D92662#2443970, @MaskRay wrote:

If the attribute is not suitable, glibc should drop it. The compiler can add readnone/readonly if appropriate.

Fix test

Fix all failing tests

Add missing file. Rename isCoroutine to isPresplitCoroutine

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

@rsmith, @v.g.vassilev hey I stamped this patch assuming it looks ok. But definitely shout at me if more feedback needs to be addressed. Happy to follow up.

In D87798#2395800, @aeubanks wrote:

Yes I'm aware of the issue and https://reviews.llvm.org/D88714 is an attempt to fix that. I was going to ask exactly what sorts of transformations coroutines does. I'll make a post in llvm-dev.

In D91305#2395900, @junparser wrote:

Thanks for the patch！

In D87798#2395800, @aeubanks wrote:

Yes I'm aware of the issue and https://reviews.llvm.org/D88714 is an attempt to fix that. I was going to ask exactly what sorts of transformations coroutines does. I'll make a post in llvm-dev.

@aeubanks, I was wondering if you are familiar with this but it seems that the SCC update after CoroSplit couldn't handle the case when there is recursion in the original coroutine, that is, the newly generated functions by CoroSplit will contain cycles (foo calling foo.resume which then calls foo). Do you have suggestions/thoughts on how to fix it?
For example, test like this will fail assertions:

; RUN: opt < %s --enable-new-pm --enable-coroutines -O3 -S
; Function Attrs: argmemonly nounwind readonly
declare token @llvm.coro.id(i32, i8* readnone, i8* nocapture readonly, i8*) #0

Do we really need handle this in non-optimized mode? @bruno I'm afraid that this patch may affect the debugging quality fixed in D90772, @lxfind, have you ever checked this?

In D91305#2392934, @junparser wrote:

I'm not sure whether we should fix this case by case. Maybe we should use memoryssa to handle this.

Landed by accident. Reverted and reopening for review.

Thank you!

Are all uses of on_ompt_callback_master expected to be replaced?
I see this is still using it:
https://github.com/llvm/llvm-project/blob/master/openmp/tools/multiplex/tests/custom_data_storage/first-tool.h#L126

Good catch. Thanks for the fix! The IsImplicit argument for buildCoawaitCalls function can also be removed now.

ping :)

Add AST test

Curious, is the plan to eventually replace .recon lowering with .async lowering? If so, would you mind explain in a bit more detail what's the advantage of .async over .retcon lowering and what motivates this new lowering? Thanks!

LGTM. Thanks!

In D90977#2383607, @ben-clayton wrote:

I can confirm that this fixes the crash we've been seeing - thank you!

In D90772#2383597, @bruno wrote:

Could you add a test (or update existing tests) to demonstrate that the issue is fixed?

The testcase was updated as part of the last diff update, anything specific you are looking for?

Could you add a test (or update existing tests) to demonstrate that the issue is fixed?