[Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish)

[ChuanqiXu]

Summary

I found that the coroutine intrinsic llvm.coro.param in documentation (https://llvm.org/docs/Coroutines.html#id101) didn't get used actually since there isn't lowering codes in LLVM. I also checked the implementation of libstdc++ and libc++. Both of them didn't use llvm.coro.param. So I am pretty sure that the llvm.coro.param intrinsic is unused. I think it would be better t to remove it to avoid possible misleading understandings.

[rjmccall]

I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the intrinsic in the frontend and then (2) checking if the copied parameter variable is actually used after reaching a suspend point, other than in ways that wouldn't happen if the intrinsic returned false.

[ChuanqiXu]

In D115222#3175577, @rjmccall wrote:

I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the intrinsic in the frontend and then (2) checking if the copied parameter variable is actually used after reaching a suspend point, other than in ways that wouldn't happen if the intrinsic returned false.

To my knowledge of language specification, the optimization is not allowed now. Both the construction and destruction is required now. But I am indeed interested to implement this. Since I know that many implementation of C++ features comes before they become standard. So I am wondering if this could be a proposal demo to the language specification : )
BTW, I am still prefer to remove the intrinsic to avoid misunderstanding. Or is It sufficient to add a TODO/FIXME marker in the Documentation? How do you think about it?

[rjmccall]

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

[ChuanqiXu]

In D115222#3177269, @rjmccall wrote:

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

Understood. I think it is not allowed and I would consult with experts in EWG to make sure.

[ChuanqiXu]

In D115222#3177269, @rjmccall wrote:

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

From the feedback from Lewis, this optimization is allowed. Here is the link: http://eel.is/c++draft/class.copy.elision#1.3. I would add it to my TOOD list. But the priority might not be high since I am working on C++20's module recently. So it might be the case that the optimization would start soon. So I think it might be better to remove it.

[rjmccall]

Okay, that's fine, I have no objection to removing it. LGTM.

clang/docs/LanguageExtensions.rst

.. code-block:: c
   void  *__builtin_coro_frame()
   void  *__builtin_coro_free(void *coro_frame)
 
   void  *__builtin_coro_id(int align, void *promise, void *fnaddr, void *parts)
   bool   __builtin_coro_alloc()
   void  *__builtin_coro_begin(void *memory)
   void   __builtin_coro_end(void *coro_frame, bool unwind)
   char   __builtin_coro_suspend(bool final)
-  bool   __builtin_coro_param(void *original, void *copy)
 
 Note that there is no builtin matching the `llvm.coro.save` intrinsic. LLVM
 automatically will insert one if the first argument to `llvm.coro.suspend` is
 token `none`. If a user calls `__builin_suspend`, clang will insert `token none`
 as the first argument to the intrinsic.
 
 Source location builtins
 ------------------------

clang/include/clang/Basic/Builtins.def

 LANGBUILTIN(__builtin_coro_noop, "v*", "n", COR_LANG)
 LANGBUILTIN(__builtin_coro_free, "v*v*", "n", COR_LANG)
 
 LANGBUILTIN(__builtin_coro_id, "v*Iiv*v*v*", "n", COR_LANG)
 LANGBUILTIN(__builtin_coro_alloc, "b", "n", COR_LANG)
 LANGBUILTIN(__builtin_coro_begin, "v*v*", "n", COR_LANG)
 LANGBUILTIN(__builtin_coro_end, "bv*Ib", "n", COR_LANG)
 LANGBUILTIN(__builtin_coro_suspend, "cIb", "n", COR_LANG)
-LANGBUILTIN(__builtin_coro_param, "bv*v*", "n", COR_LANG)
 
 // OpenCL v2.0 s6.13.16, s9.17.3.5 - Pipe functions.
 // We need the generic prototype, since the packet type could be anything.
 LANGBUILTIN(read_pipe, "i.", "tn", OCLC20_LANG)
 LANGBUILTIN(write_pipe, "i.", "tn", OCLC20_LANG)
 
 LANGBUILTIN(reserve_read_pipe, "i.", "tn", OCLC20_LANG)
 LANGBUILTIN(reserve_write_pipe, "i.", "tn", OCLC20_LANG)

clang/lib/CodeGen/CGBuiltin.cpp

   case Builtin::BI__builtin_coro_alloc:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_alloc);
   case Builtin::BI__builtin_coro_begin:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_begin);
   case Builtin::BI__builtin_coro_end:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_end);
   case Builtin::BI__builtin_coro_suspend:
     return EmitCoroutineIntrinsic(E, Intrinsic::coro_suspend);
-  case Builtin::BI__builtin_coro_param:
-    return EmitCoroutineIntrinsic(E, Intrinsic::coro_param);
 
   // OpenCL v2.0 s6.13.16.2, Built-in pipe read and write functions
   case Builtin::BIread_pipe:
   case Builtin::BIwrite_pipe: {
     Value *Arg0 = EmitScalarExpr(E->getArg(0)),
           *Arg1 = EmitScalarExpr(E->getArg(1));
     CGOpenCLRuntime OpenCLRT(CGM);
     Value *PacketSize = OpenCLRT.getPipeElemSize(E->getArg(0));

clang/test/CodeGenCoroutines/coro-builtins.c

 // RUN: %clang_cc1 -triple x86_64-pc-windows-msvc18.0.0 -fcoroutines-ts -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
 
 void *myAlloc(long long);
 
 // CHECK-LABEL: f(
 void f(int n) {
   // CHECK: %n.addr = alloca i32
-  // CHECK: %n_copy = alloca i32
   // CHECK: %promise = alloca i32
-  int n_copy;
   int promise;
 
   // CHECK: %[[PROM_ADDR:.+]] = bitcast i32* %promise to i8*
   // CHECK-NEXT: %[[COROID:.+]] = call token @llvm.coro.id(i32 32, i8* %[[PROM_ADDR]], i8* null, i8* null)
   __builtin_coro_id(32, &promise, 0, 0);
 
   // CHECK-NEXT: call i1 @llvm.coro.alloc(token %[[COROID]])
   __builtin_coro_alloc();
void f(int n) {
   // CHECK-NEXT: call i8* @llvm.coro.free(token %[[COROID]], i8* %[[FRAME]])
   __builtin_coro_free(__builtin_coro_frame());
 
   // CHECK-NEXT: call i1 @llvm.coro.end(i8* %[[FRAME]], i1 false)
   __builtin_coro_end(__builtin_coro_frame(), 0);
 
   // CHECK-NEXT: call i8 @llvm.coro.suspend(token none, i1 true)
   __builtin_coro_suspend(1);
-
-  // CHECK-NEXT: %[[N_ADDR:.+]] = bitcast i32* %n.addr to i8*
-  // CHECK-NEXT: %[[N_COPY_ADDR:.+]] = bitcast i32* %n_copy to i8*
-  // CHECK-NEXT: call i1 @llvm.coro.param(i8* %[[N_ADDR]], i8* %[[N_COPY_ADDR]])
-  __builtin_coro_param(&n, &n_copy);
 }

llvm/docs/Coroutines.rst

 abnormally (non-zero).
 
 In a normal coroutine, it is undefined behavior if the coroutine executes
 a call to ``llvm.coro.suspend.retcon`` after resuming abnormally.
 
 In a yield-once coroutine, it is undefined behavior if the coroutine
 executes a call to ``llvm.coro.suspend.retcon`` after resuming in any way.
 
-.. _coro.param:
-
-'llvm.coro.param' Intrinsic
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-::
-
-  declare i1 @llvm.coro.param(i8* <original>, i8* <copy>)
-
-Overview:
-"""""""""
-
-The '``llvm.coro.param``' is used by a frontend to mark up the code used to
-construct and destruct copies of the parameters. If the optimizer discovers that
-a particular parameter copy is not used after any suspends, it can remove the
-construction and destruction of the copy by replacing corresponding coro.param
-with `i1 false` and replacing any use of the `copy` with the `original`.
-
-Arguments:
-""""""""""
-
-The first argument points to an `alloca` storing the value of a parameter to a
-coroutine.
-
-The second argument points to an `alloca` storing the value of the copy of that
-parameter.
-
-Semantics:
-""""""""""
-
-The optimizer is free to always replace this intrinsic with `i1 true`.
-
-The optimizer is also allowed to replace it with `i1 false` provided that the
-parameter copy is only used prior to control flow reaching any of the suspend
-points. The code that would be DCE'd if the `coro.param` is replaced with
-`i1 false` is not considered to be a use of the parameter copy.
-
-The frontend can emit this intrinsic if its language rules allow for this
-optimization.
-
-Example:
-""""""""
-Consider the following example. A coroutine takes two parameters `a` and `b`
-that has a destructor and a move constructor.
-
-.. code-block:: c++
-
-  struct A { ~A(); A(A&&); bool foo(); void bar(); };
-
-  task<int> f(A a, A b) {
-    if (a.foo())
-      return 42;
-
-    a.bar();
-    co_await read_async(); // introduces suspend point
-    b.bar();
-  }
-
-Note that, uses of `b` is used after a suspend point and thus must be copied
-into a coroutine frame, whereas `a` does not have to, since it never used
-after suspend.
-
-A frontend can create parameter copies for `a` and `b` as follows:
-
-.. code-block:: text
-
-  task<int> f(A a', A b') {
-    a = alloca A;
-    b = alloca A;
-    // move parameters to its copies
-    if (coro.param(a', a)) A::A(a, A&& a');
-    if (coro.param(b', b)) A::A(b, A&& b');
-    ...
-    // destroy parameters copies
-    if (coro.param(a', a)) A::~A(a);
-    if (coro.param(b', b)) A::~A(b);
-  }
-
-The optimizer can replace coro.param(a',a) with `i1 false` and replace all uses
-of `a` with `a'`, since it is not used after suspend.
-
-The optimizer must replace coro.param(b', b) with `i1 true`, since `b` is used
-after suspend and therefore, it has to reside in the coroutine frame.
-
 Coroutine Transformation Passes
 ===============================
 CoroEarly
 ---------
 The pass CoroEarly lowers coroutine intrinsics that hide the details of the
 structure of the coroutine frame, but, otherwise not needed to be preserved to
 help later coroutine passes. This pass lowers `coro.frame`_, `coro.done`_,
 and `coro.promise`_ intrinsics.

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

InstructionCost getIntrinsicInstrCost(const IntrinsicCostAttributes &ICA,
     case Intrinsic::experimental_gc_relocate:
     case Intrinsic::coro_alloc:
     case Intrinsic::coro_begin:
     case Intrinsic::coro_free:
     case Intrinsic::coro_end:
     case Intrinsic::coro_frame:
     case Intrinsic::coro_size:
     case Intrinsic::coro_suspend:
-    case Intrinsic::coro_param:
     case Intrinsic::coro_subfn_addr:
       // These intrinsics don't actually represent code after lowering.
       return 0;
     }
     return 1;
   }
 
   InstructionCost getCallInstrCost(Function *F, Type *RetTy,

llvm/include/llvm/IR/Intrinsics.td

 def int_coro_suspend_retcon : Intrinsic<[llvm_any_ty], [llvm_vararg_ty], []>;
 def int_coro_prepare_retcon : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty],
                                         [IntrNoMem]>;
 def int_coro_alloca_alloc : Intrinsic<[llvm_token_ty],
                                       [llvm_anyint_ty, llvm_i32_ty], []>;
 def int_coro_alloca_get : Intrinsic<[llvm_ptr_ty], [llvm_token_ty], []>;
 def int_coro_alloca_free : Intrinsic<[], [llvm_token_ty], []>;
 
-def int_coro_param : Intrinsic<[llvm_i1_ty], [llvm_ptr_ty, llvm_ptr_ty],
-                               [IntrNoMem, ReadNone<ArgIndex<0>>,
-                                ReadNone<ArgIndex<1>>]>;
-
 // Coroutine Manipulation Intrinsics.
 
 def int_coro_resume : Intrinsic<[], [llvm_ptr_ty], [Throws]>;
 def int_coro_destroy : Intrinsic<[], [llvm_ptr_ty], [Throws]>;
 def int_coro_done : Intrinsic<[llvm_i1_ty], [llvm_ptr_ty],
                               [IntrArgMemOnly, ReadOnly<ArgIndex<0>>,
                                NoCapture<ArgIndex<0>>]>;
 def int_coro_promise : Intrinsic<[llvm_ptr_ty],

llvm/lib/Transforms/Coroutines/Coroutines.cpp

static const char *const CoroIntrinsics[] = {
       "llvm.coro.end.async",
       "llvm.coro.frame",
       "llvm.coro.free",
       "llvm.coro.id",
       "llvm.coro.id.async",
       "llvm.coro.id.retcon",
       "llvm.coro.id.retcon.once",
       "llvm.coro.noop",
-      "llvm.coro.param",
       "llvm.coro.prepare.async",
       "llvm.coro.prepare.retcon",
       "llvm.coro.promise",
       "llvm.coro.resume",
       "llvm.coro.save",
       "llvm.coro.size",
       "llvm.coro.subfn.addr",
       "llvm.coro.suspend",