[PATCH] D129230: [IR][coroutine] make final and non-final llvm.coro.saves unique

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D129230#3634731 <https://reviews.llvm.org/D129230#3634731>, @ychen wrote:

>> so it clearly makes no sense to merge the non-final coro saves. D129025 <https://reviews.llvm.org/D129025> looks good enough to me temporarily.
>
> Agreed.
>
>>> however, the only other choice would be to change llvm.coro.save in some way like:
>>>
>>> 1.add new flag parameter to llvm.coro.save
>>> 2.add llvm.coro.save.final
>>>
>>> I'm not a fan of both.
>>
>> So option2 is not valid. But I am a fan of option 1. Since it looks naturally to merge 2 `@llvm.coro.save(ptr null)` but it wouldn't be naturally to merge `@llvm.coro.save(ptr null, i32 0)` and `@llvm.coro.save(ptr null, i32 1)`. So we solve the problem in the IR level.
>
>
>
>> it looks naturally to merge 2 `@llvm.coro.save(ptr null)`
>
> Hmmm, I don't quite understand this. Do you please give an example?

I mean it might be a little bit surprising for people (who understand compiler but don't understand the coro intrinsics) that the optimizer wouldn't merge `@llvm.coro.save` in the example. But if the `@llvm.coro.save` intrinsic have another parameter, they won't be surprised.

> One more complication is that for
>
>   define void @f(i32 %x) {
>   entry:
>     %cmp = icmp slt i32 %x, 0
>     br i1 %cmp, label %await.suspend, label %final.suspend
>   
>   await.suspend:
>     %0 = call token @llvm.coro.save(ptr null)
>     %1 = call i8 @llvm.coro.suspend(token %0, i1 false)
>     br label %coro.ret
>   
>   final.suspend:
>     %2 = call token @llvm.coro.save(ptr null)
>     %3 = call i8 @llvm.coro.suspend(token %2, i1 false)
>     br label %coro.ret
>   
>   coro.ret:
>     ret void
>   }
>
> this is not correct (SimplifyCFG only hoists `llvm.coro.save`):
>
>   define void @f(i32 %x) {
>   entry:
>     %cmp = icmp slt i32 %x, 0
>     %0 = call token @llvm.coro.save(ptr null)
>     br i1 %cmp, label %await.suspend, label %final.suspend
>   
>   await.suspend:
>     %1 = call i8 @llvm.coro.suspend(token %0, i1 false)
>     br label %coro.ret
>   
>   final.suspend:
>     %3 = call i8 @llvm.coro.suspend(token %0, i1 false)
>     br label %coro.ret
>   
>   coro.ret:
>     ret void
>   }
>
> but this is correct (SimplifyCFG hoists `llvm.coro.suspend` too):
>
>   define void @f(i32 %x) {
>   entry:
>     %cmp = icmp slt i32 %x, 0
>     %0 = call token @llvm.coro.save(ptr null)
>     %1 = call i8 @llvm.coro.suspend(token %0, i1 false)
>     ret void
>   }

Oh, you're right. It won't be a problem if the `@llvm.coro.suspend` intrinsic get merged too.

> I think what we need is that: only allow merging `llvm.coro.save` when its user `llvm.coro.suspend` is also merged. But I don't know a good way to express that with IR.

I think you're right. I don't have a good idea too. Before we get a solution, I think the suggestion from @nikic is good enough in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129230