Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend retcon.once coroutines lowering to optionally produce a normal result #66333

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

asl
Copy link
Collaborator

@asl asl commented Sep 14, 2023

One of the main user of these kind of coroutines is swift. There yield-once (retcon.once) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, however, there is no way to represent this (as compared to switched-resume kind of coroutines where C++ co_return is transformed to a member / callback call on promise object).

The extension is simple: we allow continuation function to have a non-void result and accept optional extra arguments to llvm.coro.end intrinsic that would essentially forward them as normal results. Everything is backward compatible in terms of LLVM C++ API (as we only made llvm.coro.end intrinsic variadic), so no changes in downstream projects are expected.

@asl asl requested a review from ChuanqiXu9 September 14, 2023 06:47
@llvmbot llvmbot added clang Clang issues not falling into any other category mlir:llvm mlir coroutines C++20 coroutines llvm:ir llvm:transforms labels Sep 14, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 14, 2023

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-coroutines
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-mlir

Changes One of the main user of these kind of coroutines is swift. There yield-once (`retcon.once`) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, however, there is no way to represent this (as compared to switched-resume kind of coroutines where C++ co_return is transformed to a member / callback call on promise object).

The extension is simple: we simply allow continuation function to have non-void result and accept optional extra arguments to llvm.coro.end intrinsic that would essentially forward them as normal results. Everything is backward compatible in terms of LLVM C++ API (as we only made llvm.coro.end intrinsic variadic), so no changes in downstream projects are expected.

Patch is 132.42 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66333.diff

123 Files Affected:

  • (modified) clang/test/CodeGenCoroutines/coro-builtins.c (+1-1)
  • (modified) clang/test/CodeGenCoroutines/coro-eh-cleanup.cpp (+2-2)
  • (modified) clang/test/CodeGenCoroutines/coro-lambda.cpp (+1-1)
  • (modified) llvm/docs/Coroutines.rst (+22-7)
  • (modified) llvm/include/llvm/IR/Intrinsics.td (+1-1)
  • (modified) llvm/lib/IR/AutoUpgrade.cpp (+12)
  • (modified) llvm/lib/Transforms/Coroutines/CoroFrame.cpp (+2-1)
  • (modified) llvm/lib/Transforms/Coroutines/CoroInstr.h (+17)
  • (modified) llvm/lib/Transforms/Coroutines/CoroSplit.cpp (+25-2)
  • (modified) llvm/test/Assembler/auto_upgrade_intrinsics.ll (+7)
  • (modified) llvm/test/Transforms/Coroutines/ArgAddr.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-align16.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-align32.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-align64-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-align64.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-align8-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-align8.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloc-with-param-O0.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloc-with-param-O2.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-01.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-03.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-04.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-05.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-06.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-07.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-08.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-09.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-alloca-loop-carried-address.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-async-addr-lifetime-infinite-loop-bug.ll (+1-1)
  • (modified) llvm/test/Transforms/Coroutines/coro-async-addr-lifetime-start-bug.ll (+1-1)
  • (modified) llvm/test/Transforms/Coroutines/coro-async-dyn-align.ll (+1-1)
  • (modified) llvm/test/Transforms/Coroutines/coro-async.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-byval-param.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-catchswitch.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug-O2.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug-dbg.values-not_used_in_frame.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug-spill-dbg.declare.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-debug.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-00.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-01.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-eh-aware-edge-split-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-arrayalloca.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-00.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-01.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-03.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-04.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-reuse-alloca-05.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame-unreachable.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-frame.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-materialize.ll (+6-6)
  • (modified) llvm/test/Transforms/Coroutines/coro-noalias-param.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-padding.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-param-copy.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-preserve-final.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-readnone-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-readnone.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-alloca-opaque-ptr.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-alloca.ll (+6-6)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-frame.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-once-private.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-once-value.ll (+162-27)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-once-value2.ll (+111-8)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-opaque-ptr.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-remat.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-resume-values.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-resume-values2.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-unreachable.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon-value.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-retcon.ll (+4-4)
  • (modified) llvm/test/Transforms/Coroutines/coro-spill-after-phi.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-spill-corobegin.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-spill-defs-before-corobegin.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-spill-promise-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-spill-promise.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-00.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-01.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-alloc.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-dbg.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-eh-00.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-eh-01.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-final-suspend.ll (+4-4)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-hidden.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail-ppc64le.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail1.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail10.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail11.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail12.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail13.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail2.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail3.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail4.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail5.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail6.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail7.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail8.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-musttail9.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-no-lieftime.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-01.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-02.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-03.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-04.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/coro-swifterror.ll (+3-3)
  • (modified) llvm/test/Transforms/Coroutines/coro-zero-alloca.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/ex0.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/ex1.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/ex2.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/ex3.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/ex4.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/ex5.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/no-suspend.ll (+9-9)
  • (modified) llvm/test/Transforms/Coroutines/phi-coro-end.ll (+2-2)
  • (modified) llvm/test/Transforms/Coroutines/remarks.ll (+2-2)
  • (modified) llvm/test/Transforms/FunctionAttrs/noreturn.ll (+2-2)
  • (modified) llvm/test/Transforms/LICM/sink-with-coroutine.ll (+5-5)
  • (modified) mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir (+2-2)
diff --git a/clang/test/CodeGenCoroutines/coro-builtins.c b/clang/test/CodeGenCoroutines/coro-builtins.c
index e58820db6783987..a329200aa548dc0 100644
--- a/clang/test/CodeGenCoroutines/coro-builtins.c
+++ b/clang/test/CodeGenCoroutines/coro-builtins.c
@@ -37,7 +37,7 @@ void f(int n) {
   // CHECK-NEXT: call ptr @llvm.coro.free(token %[[COROID]], ptr %[[FRAME]])
   __builtin_coro_free(__builtin_coro_frame());
 
-  // CHECK-NEXT: call i1 @llvm.coro.end(ptr %[[FRAME]], i1 false)
+  // CHECK-NEXT: call i1 (ptr, i1, ...) @llvm.coro.end(ptr %[[FRAME]], i1 false)
   __builtin_coro_end(__builtin_coro_frame(), 0);
 
   // CHECK-NEXT: call i8 @llvm.coro.suspend(token none, i1 true)
diff --git a/clang/test/CodeGenCoroutines/coro-eh-cleanup.cpp b/clang/test/CodeGenCoroutines/coro-eh-cleanup.cpp
index c4a6ae96f551e19..e055c4d7561b2c3 100644
--- a/clang/test/CodeGenCoroutines/coro-eh-cleanup.cpp
+++ b/clang/test/CodeGenCoroutines/coro-eh-cleanup.cpp
@@ -60,7 +60,7 @@ coro_t f() {
 
 // CHECK: [[COROENDBB]]:
 // CHECK-NEXT: %[[CLPAD:.+]] = cleanuppad within none
-// CHECK-NEXT: call i1 @llvm.coro.end(ptr null, i1 true) [ "funclet"(token %[[CLPAD]]) ]
+// CHECK-NEXT: call i1 (ptr, i1, ...) @llvm.coro.end(ptr null, i1 true) [ "funclet"(token %[[CLPAD]]) ]
 // CHECK-NEXT: cleanupret from %[[CLPAD]] unwind label
 
 // CHECK-LPAD: @_Z1fv(
@@ -76,7 +76,7 @@ coro_t f() {
 // CHECK-LPAD:             to label %{{.+}} unwind label %[[UNWINDBB:.+]]
 
 // CHECK-LPAD: [[UNWINDBB]]:
-// CHECK-LPAD:   %[[I1RESUME:.+]] = call i1 @llvm.coro.end(ptr null, i1 true)
+// CHECK-LPAD:   %[[I1RESUME:.+]] = call i1 (ptr, i1, ...) @llvm.coro.end(ptr null, i1 true)
 // CHECK-LPAD:   br i1  %[[I1RESUME]], label %[[EHRESUME:.+]], label
 // CHECK-LPAD: [[EHRESUME]]:
 // CHECK-LPAD-NEXT:  %[[exn:.+]] = load ptr, ptr %exn.slot, align 8
diff --git a/clang/test/CodeGenCoroutines/coro-lambda.cpp b/clang/test/CodeGenCoroutines/coro-lambda.cpp
index 26c51070f9e2d05..06c18ddb8f640c8 100644
--- a/clang/test/CodeGenCoroutines/coro-lambda.cpp
+++ b/clang/test/CodeGenCoroutines/coro-lambda.cpp
@@ -55,4 +55,4 @@ void f() {
 //   CHECK: alloca %"struct.Task::promise_type"
 //   CHECK: call token @llvm.coro.id(
 //   CHECK: call i8 @llvm.coro.suspend(
-//   CHECK: call i1 @llvm.coro.end(
+//   CHECK: call i1 (ptr, i1, ...) @llvm.coro.end(
diff --git a/llvm/docs/Coroutines.rst b/llvm/docs/Coroutines.rst
index 0a65a39119fd83c..109d8a7f9971928 100644
--- a/llvm/docs/Coroutines.rst
+++ b/llvm/docs/Coroutines.rst
@@ -303,7 +303,7 @@ The LLVM IR for this coroutine looks like this:
     call void @free(ptr %mem)
     br label %suspend
   suspend:
-    %unused = call i1 @llvm.coro.end(ptr %hdl, i1 false)
+    %unused = call i1 (ptr, i1, ...) @llvm.coro.end(ptr %hdl, i1 false)
     ret ptr %hdl
   }
 
@@ -630,7 +630,7 @@ store the current value produced by a coroutine.
     call void @free(ptr %mem)
     br label %suspend
   suspend:
-    %unused = call i1 @llvm.coro.end(ptr %hdl, i1 false)
+    %unused = call i1 (ptr, i1, ...) @llvm.coro.end(ptr %hdl, i1 false)
     ret ptr %hdl
   }
 
@@ -1312,8 +1312,8 @@ Arguments:
 """"""""""
 
 As for ``llvm.core.id.retcon``, except that the return type of the
-continuation prototype must be `void` instead of matching the
-coroutine's return type.
+continuation prototype must represent the normal return type of the continuation
+(instead of matching the coroutine's return type).
 
 Semantics:
 """"""""""
@@ -1326,7 +1326,7 @@ A frontend should emit function attribute `presplitcoroutine` for the coroutine.
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 ::
 
-  declare i1 @llvm.coro.end(ptr <handle>, i1 <unwind>)
+  declare i1 @llvm.coro.end(ptr <handle>, i1 <unwind>, ...)
 
 Overview:
 """""""""
@@ -1347,6 +1347,21 @@ The second argument should be `true` if this coro.end is in the block that is
 part of the unwind sequence leaving the coroutine body due to an exception and
 `false` otherwise.
 
+Other arguments can only be specified for unique-suspend returned-continuation
+coroutines where they will be normal returns of a coroutine continuation
+function. The number of arguments must match the return type of the continuation
+function:
+
+- if the return type of the continuation function is ``void`` there must be no
+  extra argumets
+
+- if the return type of the continuation function is a ``struct``, the arguments
+  will be element types of that ``struct`` in order;
+
+- otherwise, it is just the return value of the continuation function.
+
+No extra arguments are allowed for coro.end calls in unwind sections
+
 Semantics:
 """"""""""
 The purpose of this intrinsic is to allow frontends to mark the cleanup and
@@ -1378,7 +1393,7 @@ For landingpad based exception model, it is expected that frontend uses the
 .. code-block:: llvm
 
     ehcleanup:
-      %InResumePart = call i1 @llvm.coro.end(ptr null, i1 true)
+      %InResumePart = call i1 (ptr, i1, ...) @llvm.coro.end(ptr null, i1 true)
       br i1 %InResumePart, label %eh.resume, label %cleanup.cont
 
     cleanup.cont:
@@ -1403,7 +1418,7 @@ referring to an enclosing cleanuppad as follows:
 
     ehcleanup:
       %tok = cleanuppad within none []
-      %unused = call i1 @llvm.coro.end(ptr null, i1 true) [ "funclet"(token %tok) ]
+      %unused = call i1 (ptr, i1, ...) @llvm.coro.end(ptr null, i1 true) [ "funclet"(token %tok) ]
       cleanupret from %tok unwind label %RestOfTheCleanup
 
 The `CoroSplit` pass, if the funclet bundle is present, will insert
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index cd6061a190fbbc0..da186dec2cbca10 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1643,7 +1643,7 @@ def int_coro_free : Intrinsic<[llvm_ptr_ty], [llvm_token_ty, llvm_ptr_ty],
                               [IntrReadMem, IntrArgMemOnly,
                                ReadOnly<ArgIndex<1>>,
                                NoCapture<ArgIndex<1>>]>;
-def int_coro_end : Intrinsic<[llvm_i1_ty], [llvm_ptr_ty, llvm_i1_ty], []>;
+def int_coro_end : Intrinsic<[llvm_i1_ty], [llvm_ptr_ty, llvm_i1_ty, llvm_vararg_ty], []>;
 def int_coro_end_async
     : Intrinsic<[llvm_i1_ty], [llvm_ptr_ty, llvm_i1_ty, llvm_vararg_ty], []>;
 
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index eedde64203e09cc..cca7649c7a61e43 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -951,6 +951,12 @@ static bool UpgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
                                         F->arg_begin()->getType());
       return true;
     }
+    if (Name.equals("coro.end") && !F->isVarArg()) {
+      rename(F);
+      NewFn = Intrinsic::getDeclaration(F->getParent(), Intrinsic::coro_end);
+      return true;
+    }
+
     break;
   }
   case 'd':
@@ -4207,6 +4213,12 @@ void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) {
     break;
   }
 
+  case Intrinsic::coro_end: {
+    SmallVector<Value *, 2> Args(CI->args());
+    NewCall = Builder.CreateCall(NewFn, Args);
+    break;
+  }
+
   case Intrinsic::vector_extract: {
     StringRef Name = F->getName();
     Name = Name.substr(5); // Strip llvm
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index a12dd710174f3f8..2ab0d7d5854d7bb 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -3046,7 +3046,8 @@ void coro::buildCoroutineFrame(
   // Collect the spills for arguments and other not-materializable values.
   for (Argument &A : F.args())
     for (User *U : A.users())
-      if (Checker.isDefinitionAcrossSuspend(A, U))
+      if (Checker.isDefinitionAcrossSuspend(A, U) ||
+          isa<CoroEndInst>(U))
         FrameData.Spills[&A].push_back(cast<Instruction>(U));
 
   const DominatorTree DT(F);
diff --git a/llvm/lib/Transforms/Coroutines/CoroInstr.h b/llvm/lib/Transforms/Coroutines/CoroInstr.h
index 014938c15a0a3e0..98fa03c68fc2e02 100644
--- a/llvm/lib/Transforms/Coroutines/CoroInstr.h
+++ b/llvm/lib/Transforms/Coroutines/CoroInstr.h
@@ -633,6 +633,23 @@ class LLVM_LIBRARY_VISIBILITY AnyCoroEndInst : public IntrinsicInst {
 /// This represents the llvm.coro.end instruction.
 class LLVM_LIBRARY_VISIBILITY CoroEndInst : public AnyCoroEndInst {
 public:
+  op_iterator retval_begin() { return std::next(arg_begin(), 2); }
+  const_op_iterator retval_begin() const { return std::next(arg_begin(), 2); }
+
+  op_iterator retval_end() { return arg_end(); }
+  const_op_iterator retval_end() const { return arg_end(); }
+
+  iterator_range<op_iterator> return_values() {
+    return make_range(retval_begin(), retval_end());
+  }
+  iterator_range<const_op_iterator> return_values() const {
+    return make_range(retval_begin(), retval_end());
+  }
+
+  unsigned numReturns() const {
+    return std::distance(retval_begin(), retval_end());
+  }
+
   // Methods to support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const IntrinsicInst *I) {
     return I->getIntrinsicID() == Intrinsic::coro_end;
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 933771c2d08984a..5b3f37a10a9d830 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -234,6 +234,8 @@ static void replaceFallthroughCoroEnd(AnyCoroEndInst *End,
   switch (Shape.ABI) {
   // The cloned functions in switch-lowering always return void.
   case coro::ABI::Switch:
+    assert(cast<CoroEndInst>(End)->numReturns() == 0 &&
+           "switch coroutine should not return any values");
     // coro.end doesn't immediately end the coroutine in the main function
     // in this lowering, because we need to deallocate the coroutine.
     if (!InResume)
@@ -251,14 +253,35 @@ static void replaceFallthroughCoroEnd(AnyCoroEndInst *End,
 
   // In unique continuation lowering, the continuations always return void.
   // But we may have implicitly allocated storage.
-  case coro::ABI::RetconOnce:
+  case coro::ABI::RetconOnce: {
     maybeFreeRetconStorage(Builder, Shape, FramePtr, CG);
-    Builder.CreateRetVoid();
+    auto *CoroEnd = cast<CoroEndInst>(End);
+    auto *RetTy = Shape.getResumeFunctionType()->getReturnType();
+    unsigned NumReturns = CoroEnd->numReturns();
+
+    if (auto ...

llvm/docs/Coroutines.rst Outdated Show resolved Hide resolved
llvm/lib/Transforms/Coroutines/CoroFrame.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the main user of these kind of coroutines is swift. There yield-once (retcon.once) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, however, there is no way to represent this (as compared to switched-resume kind of coroutines where C++ co_return is transformed to a member / callback call on promise object).

Out of curiousity, why don't we have the problem in the normal return continuation ABI?

llvm/lib/Transforms/Coroutines/CoroFrame.cpp Outdated Show resolved Hide resolved
@asl
Copy link
Collaborator Author

asl commented Sep 14, 2023

Out of curiousity, why don't we have the problem in the normal return continuation ABI?

The problem happens when the value is directly used in coro.end intrinsic. For example, when we're forwarding coroutine argument as a result. Or, when the value itself is computed before the suspend. Everything else is correctly handled by the present code due to BB split (the corresponding instructions appear in Cleanup block for example and correctly spilled).

@ChuanqiXu9
Copy link
Member

Out of curiousity, why don't we have the problem in the normal return continuation ABI?

The problem happens when the value is directly used in coro.end intrinsic. For example, when we're forwarding coroutine argument as a result. Or, when the value itself is computed before the suspend. Everything else is correctly handled by the present code due to BB split (the corresponding instructions appear in Cleanup block for example and correctly spilled).

I still don't understand the motivation fully. Do you say we don't have the problem naturally? Or could you show some motivation examples? (In LLVM IR?)

@asl
Copy link
Collaborator Author

asl commented Sep 15, 2023

Out of curiousity, why don't we have the problem in the normal return continuation ABI?

The problem happens when the value is directly used in coro.end intrinsic. For example, when we're forwarding coroutine argument as a result. Or, when the value itself is computed before the suspend. Everything else is correctly handled by the present code due to BB split (the corresponding instructions appear in Cleanup block for example and correctly spilled).

I still don't understand the motivation fully. Do you say we don't have the problem naturally? Or could you show some motivation examples? (In LLVM IR?)

This one is problematic:

define {ptr, ptr} @g(ptr %buffer, ptr %ptr, i8 %val) presplitcoroutine {
entry:
  %temp = alloca i32, align 4
  %id = call token @llvm.coro.id.retcon.once(i32 8, i32 8, ptr %buffer, ptr @prototype2, ptr @allocate, ptr @deallocate)
  %hdl = call ptr @llvm.coro.begin(token %id, ptr null)
  %oldvalue = load i32, ptr %ptr
  store i32 %oldvalue, ptr %temp
  %unwind = call i1 (...) @llvm.coro.suspend.retcon.i1(ptr %temp)
  br i1 %unwind, label %cleanup, label %cont

cont:
  %newvalue = load i32, ptr %temp
  store i32 %newvalue, ptr %ptr
  br label %cleanup

cleanup:
  call i1 (ptr, i1, ...) @llvm.coro.end(ptr %hdl, i1 0, i8 %val)
  unreachable
}

but this one is not (cleanup will be split before coro.end and %newval will be spilled):

define {ptr, ptr} @g(ptr %buffer, ptr %ptr, i8 %val) presplitcoroutine {
entry:
  %temp = alloca i32, align 4
  %id = call token @llvm.coro.id.retcon.once(i32 8, i32 8, ptr %buffer, ptr @prototype2, ptr @allocate, ptr @deallocate)
  %hdl = call ptr @llvm.coro.begin(token %id, ptr null)
  %oldvalue = load i32, ptr %ptr
  store i32 %oldvalue, ptr %temp
  %unwind = call i1 (...) @llvm.coro.suspend.retcon.i1(ptr %temp)
  br i1 %unwind, label %cleanup, label %cont

cont:
  %newvalue = load i32, ptr %temp
  store i32 %newvalue, ptr %ptr
  br label %cleanup

cleanup:
  %newval = add i8 %val, 42
  call i1 (ptr, i1, ...) @llvm.coro.end(ptr %hdl, i1 0, i8 %newval)
  unreachable
}

This one is problematic as well:

define {ptr, ptr} @g(ptr %buffer, ptr %ptr, i8 %val) presplitcoroutine {
entry:
  %temp = alloca i32, align 4
  %id = call token @llvm.coro.id.retcon.once(i32 8, i32 8, ptr %buffer, ptr @prototype2, ptr @allocate, ptr @deallocate)
  %hdl = call ptr @llvm.coro.begin(token %id, ptr null)
  %oldvalue = load i32, ptr %ptr
  store i32 %oldvalue, ptr %temp
  %newval = add i8 %val, 42
  %unwind = call i1 (...) @llvm.coro.suspend.retcon.i1(ptr %temp)
  br i1 %unwind, label %cleanup, label %cont

cont:
  %newvalue = load i32, ptr %temp
  store i32 %newvalue, ptr %ptr
  br label %cleanup

cleanup:
  call i1 (ptr, i1, ...) @llvm.coro.end(ptr %hdl, i1 0, i8 %newval)
  unreachable
}

All these are "new" cases I would say, everything else is handled via current split approach.

@ChuanqiXu9
Copy link
Member

Oh, I am wondering if we have misunderstandings. I am not asking the reason why we don't have the problem which is discussing in the change of CoroFrame.cpp in return continuation ABI. I understand that fully.

What make me curious is the motivation case of the PR. I mean what can be presented in retcon.once ABI after the PR which is impossible/hard before. And how do we handle that in retcon ABI.

Sorry for confusion.

@asl
Copy link
Collaborator Author

asl commented Sep 15, 2023

What make me curious is the motivation case of the PR. I mean what can be presented in retcon.once ABI after the PR which is impossible/hard before. And how do we handle that in retcon ABI.

Well, the PR allows recon.once coroutines to have normal results in addition to yields. While it might be possible to "emulate" this functionality returning the value indirectly, it is not very convenient for the producer (instead of just returning the value we'd need to allocate stack slot, pass the address, etc.) and might incur some overhead, as we'd essentially will need to capture both value to be returned and return address in the coroutine frame only to emit the store in the continuation part.

The particular usecase from Swift is as follows:

  • Yield pointer to some internals of an object
  • Allow the caller to modify the object via exposed pointer as necessary
  • In the coroutine continuation perform some "finalization" and return e.g. a pointer to a closure object with modified object being captured

The particular example of coroutine lowering could be seen in https://github.com/llvm/llvm-project/pull/66333/files#diff-70da911ce77807c88cdfcf3578aa2a569e131c99653488d148a5fd6e9b0df169

@ChuanqiXu9
Copy link
Member

Got it. Thanks. Then I am wondering how about the retcon ABI? I am confused since the doc said the last continuation of retcon ABI will return a nullptr to indicate it has finished. Don't we have the same problem? Or we can't solve the problem in retcon ABI? Or we just don't want to solve that in the current PR?

@asl
Copy link
Collaborator Author

asl commented Sep 15, 2023

Got it. Thanks. Then I am wondering how about the retcon ABI? I am confused since the doc said the last continuation of retcon ABI will return a nullptr to indicate it has finished. Don't we have the same problem? Or we can't solve the problem in retcon ABI? Or we just don't want to solve that in the current PR?

Ah, ok. The current PR is only for retcon.once coroutines. I do not have a good use-case for retcon ones and they are lowered differently enough, so I decided not to touch them. If / when there will be a viable usecase, I will try to generalize the present implementation to generic retcon coroutines.

@ChuanqiXu9
Copy link
Member

Got it. Thanks for the explanation : )

Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@asl asl merged commit 51d5d7b into llvm:main Sep 15, 2023
1 of 2 checks passed
@asl asl deleted the coro-retcon-once-return branch September 15, 2023 16:54
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
…al result (llvm#66333)

One of the main user of these kind of coroutines is swift. There yield-once (`retcon.once`) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, but there is no convenient way to represent this (as compared to switched-resume kind of coroutines where C++ `co_return`
is transformed to a member / callback call on promise object).

The extension is simple: we allow continuation function to have a non-void result and accept optional extra arguments via a special `llvm.coro.end.result` intrinsic that would essentially forward them as normal results.
asl added a commit to asl/llvm-project that referenced this pull request Feb 5, 2024
…al result (llvm#66333)

One of the main user of these kind of coroutines is swift. There yield-once (`retcon.once`) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, but there is no convenient way to represent this (as compared to switched-resume kind of coroutines where C++ `co_return`
is transformed to a member / callback call on promise object).

The extension is simple: we allow continuation function to have a non-void result and accept optional extra arguments via a special `llvm.coro.end.result` intrinsic that would essentially forward them as normal results.
asl added a commit to asl/llvm-project that referenced this pull request Mar 22, 2024
…al result (llvm#66333)

One of the main user of these kind of coroutines is swift. There yield-once (`retcon.once`) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, but there is no convenient way to represent this (as compared to switched-resume kind of coroutines where C++ `co_return` is transformed to a member / callback call on promise object).

The extension is simple: we allow continuation function to have a non-void result and accept optional extra arguments via a special `llvm.coro.end.result` intrinsic that would essentially forward them as normal results.
rxwei pushed a commit to apple/llvm-project that referenced this pull request Mar 27, 2024
* Update coroutine intrinsics documentation and few remaining tests to opaque pointers (llvm#65698)

* Extend `retcon.once` coroutines lowering to optionally produce a normal result (llvm#66333)

One of the main user of these kind of coroutines is swift. There yield-once (`retcon.once`) coroutines are used to temporary "expose" pointers to internal fields of various objects creating borrow scopes.

However, in some cases it might be useful also to allow these coroutines to produce a normal result, but there is no convenient way to represent this (as compared to switched-resume kind of coroutines where C++ `co_return` is transformed to a member / callback call on promise object).

The extension is simple: we allow continuation function to have a non-void result and accept optional extra arguments via a special `llvm.coro.end.result` intrinsic that would essentially forward them as normal results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category coroutines C++20 coroutines llvm:ir llvm:transforms mlir:llvm mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants