[Coroutines] Don't add musttail call if WebAssembly are enabled

[ChuanqiXu]

Summary

The C++20 Coroutines couldn't be compiled to WebAssembly due to an optimization named symmetric transfer requires the support for musttail calls but WebAssembly doesn't support it yet. So the revision tried to disable the optimization for WebAssembly to make we could compile standard C++20 codes in that platform.

[tlively]

Thanks for taking a look at this!

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1570

It would be good if we could call out to the TargetTransformInfo to ask the backend whether it supports tail calls. Then we could gracefully handle the case where the WebAssembly tail-call target feature is enabled.

[avogelsgesang]

Afaict, symmetric transfer is guaranteed by the C++20 standard.

https://eel.is/c++draft/expr.await#note-1 states

[...] Any number of coroutines can be successively resumed in this fashion, eventually returning control flow to the current coroutine caller or resumer [...]

Without symmetric transfer or an equivalent optimization, this statement wouldn't hold true, because we would run out of stack space before resuming an arbitrary number of coroutines

[avogelsgesang]

Also, note that cppcoro (afaict, the de-facto reference implementation for C++20 coroutine types) mentions in a comment, that

[...] the compiler supports returning a coroutine_handle from the await_suspend() method as a way of transferring execution to another coroutine with a guaranteed tail-call.

(emphasis mine)

[ChuanqiXu]

Fix typos.

[ChuanqiXu]

In D128163#3599994, @avogelsgesang wrote:

Afaict, symmetric transfer is guaranteed by the C++20 standard.

https://eel.is/c++draft/expr.await#note-1 states

[...] Any number of coroutines can be successively resumed in this fashion, eventually returning control flow to the current coroutine caller or resumer [...]

Without symmetric transfer or an equivalent optimization, this statement wouldn't hold true, because we would run out of stack space before resuming an arbitrary number of coroutines

Oh, my bad. Thanks for reporting this. But it is still odd to me since it is not supported by all archs and C++ is intended to be target independent. I've sent this question to WG21. I would take an eye on this. And let's not block this one to not prevent C++ coroutines to be used in more targets.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

1570

I tried to look TargetTransformInfo at first but I don't find the wanted methods. Then I don't find similar things in CodeGen and Target as well. So it looks like there is not on-the-shelf tool. I prefer the current method as a workaround and replace this workaround later if we or anyone implemented thing interface we want.

[ChuanqiXu]

Rebase.

[tlively]

I think it would make sense to add a new method to TTI for this, since hardcoding an exception for Wasm is not going to be future-proof at all. The tail-call target feature, while not standard yet, is very close to being standardized and shipped in browsers. It's already available behind a flag in Chrome and Node. Also, if possible, it would be good to perform a different transformation that can still do symmetric transfer without tail calls if the prevailing view is that symmetric transfer is guaranteed by the spec (but simply omitting musttail for now seems like a reasonable stopgap measure).

[ChuanqiXu]

In D128163#3613288, @tlively wrote:

I think it would make sense to add a new method to TTI for this, since hardcoding an exception for Wasm is not going to be future-proof at all. The tail-call target feature, while not standard yet, is very close to being standardized and shipped in browsers. It's already available behind a flag in Chrome and Node.

Yeah, it makes sense.

Also, if possible, it would be good to perform a different transformation that can still do symmetric transfer without tail calls if the prevailing view is that symmetric transfer is guaranteed by the spec (but simply omitting musttail for now seems like a reasonable stopgap measure).

Maybe it is possible to perform symmetric transfer without tail call. But the complexity of implementation and efficiency may be a problem. So the first step might omit it simply and I'll file an issue for it.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

if (!SI->isFinal() && simplifySuspendPoint(SI, Shape.CoroBegin)) {
       continue;
     }
     if (++I == N)
       break;
   }
   S.resize(N);
 }
 
+static bool isTargetSupportMusttail(Module *M) {
+  // Wasm tail calls are not yet standard and have not shipped on any major
+  // WebAssembly engines.
+  return !Triple(M->getTargetTriple()).isWasm();
+}
+
 static void splitSwitchCoroutine(Function &F, coro::Shape &Shape,
                                  SmallVectorImpl<Function *> &Clones) {
   assert(Shape.ABI == coro::ABI::Switch);
 
   createResumeEntryBlock(F, Shape);
   auto ResumeClone = createClone(F, ".resume", Shape,
                                  CoroCloner::Kind::SwitchResume);
   auto DestroyClone = createClone(F, ".destroy", Shape,
                                   CoroCloner::Kind::SwitchUnwind);
   auto CleanupClone = createClone(F, ".cleanup", Shape,
                                   CoroCloner::Kind::SwitchCleanup);
 
   postSplitCleanup(*ResumeClone);
   postSplitCleanup(*DestroyClone);
   postSplitCleanup(*CleanupClone);
 
+  // The implementation of symmetric transfer requires the backend support
+  // musttail calls. So we don't do it for targets which don't support
+  // musttail calls as a workaround.
+  if (isTargetSupportMusttail(F.getParent()))
     addMustTailToCoroResumes(*ResumeClone);
 
   // Store addresses resume/destroy/cleanup functions in the coroutine frame.
   updateCoroFrame(Shape, ResumeClone, DestroyClone, CleanupClone);
-
   assert(Clones.empty());
   Clones.push_back(ResumeClone);
   Clones.push_back(DestroyClone);
   Clones.push_back(CleanupClone);
 
   // Create a constant array referring to resume/destroy/clone functions pointed
   // by the last argument of @llvm.coro.info, so that CoroElide pass can
   // determined correct function to call.

llvm/test/Transforms/Coroutines/coro-split-musttail8.ll

+; Tests that we wouldn't convert coro.resume to a musttail call if the target is
+; Wasm64.
+; RUN: opt < %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s
+
+target triple = "wasm64-unknown-unknown"
+
+define void @f() #0 {
+entry:
+  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
+  %alloc = call i8* @malloc(i64 16) #3
+  %vFrame = call noalias nonnull i8* @llvm.coro.begin(token %id, i8* %alloc)
+
+  %save = call token @llvm.coro.save(i8* null)
+  %addr1 = call i8* @llvm.coro.subfn.addr(i8* null, i8 0)
+  %pv1 = bitcast i8* %addr1 to void (i8*)*
+  call fastcc void %pv1(i8* null)
+
+  %suspend = call i8 @llvm.coro.suspend(token %save, i1 false)
+  switch i8 %suspend, label %exit [
+    i8 0, label %await.ready
+    i8 1, label %exit
+  ]
+await.ready:
+  %save2 = call token @llvm.coro.save(i8* null)
+  %addr2 = call i8* @llvm.coro.subfn.addr(i8* null, i8 0)
+  %pv2 = bitcast i8* %addr2 to void (i8*)*
+  call fastcc void %pv2(i8* null)
+
+  %suspend2 = call i8 @llvm.coro.suspend(token %save2, i1 false)
+  switch i8 %suspend2, label %exit [
+    i8 0, label %exit
+    i8 1, label %exit
+  ]
+exit:
+  call i1 @llvm.coro.end(i8* null, i1 false)
+  ret void
+}
+
+; CHECK-NOT: musttail
+
+declare token @llvm.coro.id(i32, i8* readnone, i8* nocapture readonly, i8*) #1
+declare i1 @llvm.coro.alloc(token) #2
+declare i64 @llvm.coro.size.i64() #3
+declare i8* @llvm.coro.begin(token, i8* writeonly) #2
+declare token @llvm.coro.save(i8*) #2
+declare i8* @llvm.coro.frame() #3
+declare i8 @llvm.coro.suspend(token, i1) #2
+declare i8* @llvm.coro.free(token, i8* nocapture readonly) #1
+declare i1 @llvm.coro.end(i8*, i1) #2
+declare i8* @llvm.coro.subfn.addr(i8* nocapture readonly, i8) #1
+declare i8* @malloc(i64)
+
+attributes #0 = { presplitcoroutine }
+attributes #1 = { argmemonly nounwind readonly }
+attributes #2 = { nounwind }
+attributes #3 = { nounwind readnone }

llvm/test/Transforms/Coroutines/coro-split-musttail9.ll

+; Tests that we wouldn't convert coro.resume to a musttail call if the target is
+; Wasm32.
+; RUN: opt < %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s
+
+target triple = "wasm32-unknown-unknown-wasm"
+
+define void @f() #0 {
+entry:
+  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
+  %alloc = call i8* @malloc(i64 16) #3
+  %vFrame = call noalias nonnull i8* @llvm.coro.begin(token %id, i8* %alloc)
+
+  %save = call token @llvm.coro.save(i8* null)
+  %addr1 = call i8* @llvm.coro.subfn.addr(i8* null, i8 0)
+  %pv1 = bitcast i8* %addr1 to void (i8*)*
+  call fastcc void %pv1(i8* null)
+
+  %suspend = call i8 @llvm.coro.suspend(token %save, i1 false)
+  switch i8 %suspend, label %exit [
+    i8 0, label %await.ready
+    i8 1, label %exit
+  ]
+await.ready:
+  %save2 = call token @llvm.coro.save(i8* null)
+  %addr2 = call i8* @llvm.coro.subfn.addr(i8* null, i8 0)
+  %pv2 = bitcast i8* %addr2 to void (i8*)*
+  call fastcc void %pv2(i8* null)
+
+  %suspend2 = call i8 @llvm.coro.suspend(token %save2, i1 false)
+  switch i8 %suspend2, label %exit [
+    i8 0, label %exit
+    i8 1, label %exit
+  ]
+exit:
+  call i1 @llvm.coro.end(i8* null, i1 false)
+  ret void
+}
+
+; CHECK-NOT: musttail
+
+declare token @llvm.coro.id(i32, i8* readnone, i8* nocapture readonly, i8*) #1
+declare i1 @llvm.coro.alloc(token) #2
+declare i64 @llvm.coro.size.i64() #3
+declare i8* @llvm.coro.begin(token, i8* writeonly) #2
+declare token @llvm.coro.save(i8*) #2
+declare i8* @llvm.coro.frame() #3
+declare i8 @llvm.coro.suspend(token, i1) #2
+declare i8* @llvm.coro.free(token, i8* nocapture readonly) #1
+declare i1 @llvm.coro.end(i8*, i1) #2
+declare i8* @llvm.coro.subfn.addr(i8* nocapture readonly, i8) #1
+declare i8* @malloc(i64)
+
+attributes #0 = { presplitcoroutine }
+attributes #1 = { argmemonly nounwind readonly }
+attributes #2 = { nounwind }
+attributes #3 = { nounwind readnone }