[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/asl resolved https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/asl review_requested https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
llvmbot wrote: @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
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/asl edited https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
ChuanqiXu9 wrote: It looks like we need to touch the section `Returned-Continuation Lowering` too https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
@@ -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(U)) ChuanqiXu9 wrote: We do we need to include CoroEnd explicitly here? https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/ChuanqiXu9 edited https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/ChuanqiXu9 commented: > 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? https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
@@ -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(U)) ChuanqiXu9 wrote: Got it. Then it smells slightly bad. We have a lot `isDefinitionAcrossSuspend` uses and it looks easy to forget similar cases. I feel better to solve the issue fundamentally by introducing new intrinsics. e.g., we can introduce `token llvm.coro.end.retcont.return_values(...)` and refactor `llvm.coro.end()` to `declare i1 @llvm.coro.end(ptr , i1 , token )`. And for non-retcon-once ABI, we can fill the third argument as `none` simply. For retcon-once ABI, it should be easy to generate the actual ret too. Then we can avoid the `isDefinitionAcrossSuspend` problem automatically since the BB will be split around `coro.end`. https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
asl wrote: > 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). https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
@@ -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(U)) asl wrote: Ok, I'll give it a try. Note that this will be a C++ API breaking change though as we will always need to pass an extra argument. https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
@@ -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(U)) ChuanqiXu9 wrote: Thanks. The API breaking is relatively acceptable since generally we don't have requirement for backport compatibility. https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
ChuanqiXu9 wrote: > > 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?) https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
asl wrote:
> > > 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:
```llvm
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:
```llvm
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:
```llvm
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.
https://github.com/llvm/llvm-project/pull/66333
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
ChuanqiXu9 wrote: 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. https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
asl wrote: > 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 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 https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
ChuanqiXu9 wrote: 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? https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
asl wrote: > 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. https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
ChuanqiXu9 wrote: Got it. Thanks for the explanation : ) https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/ChuanqiXu9 approved this pull request. LGTM. Thanks. https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Extend `retcon.once` coroutines lowering to optionally produce a normal result (PR #66333)
https://github.com/asl closed https://github.com/llvm/llvm-project/pull/66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
