[PATCH] D136932: [Coroutines] Use default attributes for some coro intrinsics

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

In D136932#3891400 <https://reviews.llvm.org/D136932#3891400>, @nikic wrote:

> In D136932#3891356 <https://reviews.llvm.org/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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136932/new/

https://reviews.llvm.org/D136932