[Coroutines] Use default attributes for some coro intrinsics

[nikic]

Summary

This adds the default attributes (nosync, nofree, nocallback, willreturn) to the coro.id and coro.subfn.addr intrinsics. This is needed to avoid optimization regressions in the future.

It's probably possible to use default attributes for most other coro intrinsics as well, but I only hit these as problematic in practice.

[ChuanqiXu]

Thanks for doing this! I'll try to look at the other intrinsics. For this patch, coro.id looks OK to be marked as Default. But llvm.coro.subfn.addr can't be marked as nofree and I am not sure if it is good to be marked as nosync. Since coro.subfn.addr comes from either coro.destroy(Frame*) or coro.resume(Frame*). And destroy(Frame*) is clearly not good for nofree. And we can think coro.resume(Frame*) is general call to any function (if we don't apply IPA). So it looks not good to mark it as nosync if I don't misunderstand nosync.

[nikic]

In D136932#3891356, @ChuanqiXu wrote:

Thanks for doing this! I'll try to look at the other intrinsics. For this patch, coro.id looks OK to be marked as Default. But llvm.coro.subfn.addr can't be marked as nofree and I am not sure if it is good to be marked as nosync. Since coro.subfn.addr comes from either coro.destroy(Frame*) or coro.resume(Frame*). And destroy(Frame*) is clearly not good for nofree. And we can think coro.resume(Frame*) is general call to any function (if we don't apply IPA). So it looks not good to mark it as nosync if I don't misunderstand nosync.

I was looking at this lowering of coro.subfn.addr: https://github.com/llvm/llvm-project/blob/dc39819915eeba7fa26663fddedf4ed90748ee0f/llvm/lib/Transforms/Coroutines/CoroCleanup.cpp#L30-L45 It seems to be lowered to a simple (non-atomic) load at an offset of the frame, so I would expect it to be both nofree and nosync.

[ChuanqiXu]

In D136932#3891400, @nikic wrote:

In D136932#3891356, @ChuanqiXu wrote:

Thanks for doing this! I'll try to look at the other intrinsics. For this patch, coro.id looks OK to be marked as Default. But llvm.coro.subfn.addr can't be marked as nofree and I am not sure if it is good to be marked as nosync. Since coro.subfn.addr comes from either coro.destroy(Frame*) or coro.resume(Frame*). And destroy(Frame*) is clearly not good for nofree. And we can think coro.resume(Frame*) is general call to any function (if we don't apply IPA). So it looks not good to mark it as nosync if I don't misunderstand nosync.

I was looking at this lowering of coro.subfn.addr: https://github.com/llvm/llvm-project/blob/dc39819915eeba7fa26663fddedf4ed90748ee0f/llvm/lib/Transforms/Coroutines/CoroCleanup.cpp#L30-L45 It seems to be lowered to a simple (non-atomic) load at an offset of the frame, so I would expect it to be both nofree and nosync.

Oh, you're right. llvm.coro.subfn comes from coro.destroy and coro.resume. But it doesn't come from that directly. A coro.resume call would be lowered to:

%0 = call @llvm.coro.subf... // which is a load actually.
%1 = bitcast %0 to ...
%2 = call %1(...)

So I took the incorrect memory and you're right. Thanks!

llvm/include/llvm/IR/Intrinsics.td

def int_experimental_gc_get_pointer_offset : Intrinsic<[llvm_i64_ty],
                  [llvm_anyptr_ty], [IntrNoMem, IntrWillReturn,
                  ReadNone<ArgIndex<0>>, NoCapture<ArgIndex<0>>]>;
 
 //===------------------------ Coroutine Intrinsics ---------------===//
 // These are documented in docs/Coroutines.rst
 
 // Coroutine Structure Intrinsics.
 
-def int_coro_id : Intrinsic<[llvm_token_ty], [llvm_i32_ty, llvm_ptr_ty,
-                             llvm_ptr_ty, llvm_ptr_ty],
-                            [IntrArgMemOnly, IntrReadMem,
-                             ReadNone<ArgIndex<1>>, ReadOnly<ArgIndex<2>>,
-                             NoCapture<ArgIndex<2>>]>;
+def int_coro_id : DefaultAttrsIntrinsic<[llvm_token_ty],
+    [llvm_i32_ty, llvm_ptr_ty, llvm_ptr_ty, llvm_ptr_ty],
+    [IntrArgMemOnly, IntrReadMem, ReadNone<ArgIndex<1>>, ReadOnly<ArgIndex<2>>,
 def int_coro_id_retcon : Intrinsic<[llvm_token_ty],
     [llvm_i32_ty, llvm_i32_ty, llvm_ptr_ty,
      llvm_ptr_ty, llvm_ptr_ty, llvm_ptr_ty],
     []>;
 def int_coro_id_retcon_once : Intrinsic<[llvm_token_ty],
     [llvm_i32_ty, llvm_i32_ty, llvm_ptr_ty,
      llvm_ptr_ty, llvm_ptr_ty, llvm_ptr_ty],
     []>;
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_ptr_ty, llvm_i32_ty, llvm_i1_ty],
                                  [IntrNoMem, NoCapture<ArgIndex<0>>]>;
 
 // Coroutine Lowering Intrinsics. Used internally by coroutine passes.
 
-def int_coro_subfn_addr : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty, llvm_i8_ty],
-                                    [IntrReadMem, IntrArgMemOnly,
-                                     ReadOnly<ArgIndex<0>>,
-                                     NoCapture<ArgIndex<0>>]>;
+def int_coro_subfn_addr : DefaultAttrsIntrinsic<
+    [llvm_ptr_ty], [llvm_ptr_ty, llvm_i8_ty],
+    [IntrReadMem, IntrArgMemOnly, ReadOnly<ArgIndex<0>>,
 
 ///===-------------------------- Other Intrinsics --------------------------===//
 //
 def int_trap : Intrinsic<[], [], [IntrNoReturn, IntrCold]>,
                ClangBuiltin<"__builtin_trap">;
 def int_debugtrap : Intrinsic<[]>,
                     ClangBuiltin<"__builtin_debugtrap">;
 def int_ubsantrap : Intrinsic<[], [llvm_i8_ty],