Re: [PATCH] coroutines: Implement n4849 recommended symmetric transfer.
On 3/24/20 2:08 PM, Iain Sandoe wrote: tree suspend = TREE_VEC_ELT (awaiter_calls, 1); /* await_suspend(). */ + tree susp_type; + if (tree fndecl = cp_get_callee_fndecl_nofold (suspend)) +susp_type = TREE_TYPE (TREE_TYPE (fndecl)); + else +susp_type = TREE_TYPE (suspend); Why, when there's a call of a named function, is it that TREE_TYPE (suspend) is insufficient? You mentioned TARGET_EXPR, but why is their type differenty? @@ -2012,6 +2018,12 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody, = create_named_label_with_ctx (loc, "actor.begin", actor); tree actor_frame = build1_loc (loc, INDIRECT_REF, coro_frame_type, actor_fp); + /* Declare the continuation handle. */ + tree ci = build_stmt (loc, DECL_EXPR, continuation); + ci = build1_loc (loc, CONVERT_EXPR, void_type_node, ci); + ci = maybe_cleanup_point_expr_void (ci); + add_stmt (ci); you don't need to wrap in a CONVERT_EXPR, can you use add_decl_expr ? @@ -2368,6 +2383,35 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody, + + /* Now we have the actual call, and we can mark it as a tail. */ + CALL_EXPR_TAILCALL (resume) = true; + /* ... and for optimisation levels 0..1, mark it as requiring a tail-call + for correctness. It seems that doing this for optimisation levels that + normally perform tail-calling, confuses the ME (or it would be logical + to put this on unilaterally). */ + if (optimize < 2) +CALL_EXPR_MUST_TAIL_CALL (resume) = true; + resume = coro_build_cvt_void_expr_stmt (resume, loc); I'd be happier with gcc_checking_assert (maybe_cleanup_point_expr_void (resume) == resume); here. I see we can wrap RETURN_EXPRs with cleanups, so perhaps adding is ok anyway? nathan -- Nathan Sidwell
Re: [PATCH] coroutines: Implement n4849 recommended symmetric transfer.
Nathan Sidwell wrote:
On 3/24/20 2:08 PM, Iain Sandoe wrote:
Hi Nathan,
Thanks for the review,
comments embedded and a new version attached.
@David, you added the CALL_EXPR_MUST_TAIL_CALL which I’ve made use of
here (but with an observation). Perhaps you would be able to comment on
whether
I’ve (ab-)used it correctly?
OK for master now?
thanks
Iain
Nathan Sidwell wrote:
On 3/20/20 11:40 AM, Iain Sandoe via Gcc-patches wrote:
tree suspend = TREE_VEC_ELT (awaiter_calls, 1); /* await_suspend(). */
+ tree susp_type;
+ if (TREE_CODE (suspend) == CALL_EXPR)
+{
+ susp_type = CALL_EXPR_FN (suspend);
+ if (TREE_CODE (susp_type) == ADDR_EXPR)
+ susp_type = TREE_OPERAND (susp_type, 0);
+ susp_type = TREE_TYPE (TREE_TYPE (susp_type));
+}
+ else
+susp_type = TREE_TYPE (suspend);
I think:
if (tree fndec = get_callee_fndecl_nofold (suspend))
susp_type = TREE_TYPE (TREE_TYPE (fndecl));
else
susp_type = TREE_TYPE (suspend);
would do? But how can TREE_TYPE (suspend) be different from the return
type of the function decl? It seems funky that the behaviour could
depend on the form of the suspend. What am I missing?
Your proposed change is much neater, thanks.
The reason for the alternate is that the expression can also be a
target_expr.
That's not clarifying it for me. Why is a CALL_EXPR's type different to
they return type of the function being called? And why in that case do
you want the former, not the latter?
it isn’t (and therefore I don’t), and the type of the call expr is fine viz:
tree susp_type = TREE_TYPE (suspend);
(changed locally)
Iain
Re: [PATCH] coroutines: Implement n4849 recommended symmetric transfer.
On 3/24/20 2:08 PM, Iain Sandoe wrote:
Hi Nathan,
Thanks for the review,
comments embedded and a new version attached.
@David, you added the CALL_EXPR_MUST_TAIL_CALL which I’ve made use of
here (but with an observation). Perhaps you would be able to comment on whether
I’ve (ab-)used it correctly?
OK for master now?
thanks
Iain
Nathan Sidwell wrote:
On 3/20/20 11:40 AM, Iain Sandoe via Gcc-patches wrote:
tree suspend = TREE_VEC_ELT (awaiter_calls, 1); /* await_suspend(). */
+ tree susp_type;
+ if (TREE_CODE (suspend) == CALL_EXPR)
+{
+ susp_type = CALL_EXPR_FN (suspend);
+ if (TREE_CODE (susp_type) == ADDR_EXPR)
+ susp_type = TREE_OPERAND (susp_type, 0);
+ susp_type = TREE_TYPE (TREE_TYPE (susp_type));
+}
+ else
+susp_type = TREE_TYPE (suspend);
I think:
if (tree fndec = get_callee_fndecl_nofold (suspend))
susp_type = TREE_TYPE (TREE_TYPE (fndecl));
else
susp_type = TREE_TYPE (suspend);
would do? But how can TREE_TYPE (suspend) be different from the return type of
the function decl? It seems funky that the behaviour could depend on the form
of the suspend. What am I missing?
Your proposed change is much neater, thanks.
The reason for the alternate is that the expression can also be a target_expr.
That's not clarifying it for me. Why is a CALL_EXPR's type different to
they return type of the function being called? And why in that case do
you want the former, not the latter?
nathan
--
Nathan Sidwell
Re: [PATCH] coroutines: Implement n4849 recommended symmetric transfer.
Hi Nathan,
Thanks for the review,
comments embedded and a new version attached.
@David, you added the CALL_EXPR_MUST_TAIL_CALL which I’ve made use of
here (but with an observation). Perhaps you would be able to comment on whether
I’ve (ab-)used it correctly?
OK for master now?
thanks
Iain
Nathan Sidwell wrote:
> On 3/20/20 11:40 AM, Iain Sandoe via Gcc-patches wrote:
>
>>tree suspend = TREE_VEC_ELT (awaiter_calls, 1); /* await_suspend(). */
>> + tree susp_type;
>> + if (TREE_CODE (suspend) == CALL_EXPR)
>> +{
>> + susp_type = CALL_EXPR_FN (suspend);
>> + if (TREE_CODE (susp_type) == ADDR_EXPR)
>> +susp_type = TREE_OPERAND (susp_type, 0);
>> + susp_type = TREE_TYPE (TREE_TYPE (susp_type));
>> +}
>> + else
>> +susp_type = TREE_TYPE (suspend);
>
> I think:
> if (tree fndec = get_callee_fndecl_nofold (suspend))
>susp_type = TREE_TYPE (TREE_TYPE (fndecl));
> else
>susp_type = TREE_TYPE (suspend);
> would do? But how can TREE_TYPE (suspend) be different from the return type
> of the function decl? It seems funky that the behaviour could depend on the
> form of the suspend. What am I missing?
Your proposed change is much neater, thanks.
The reason for the alternate is that the expression can also be a target_expr.
>> @@ -1532,6 +1553,8 @@ co_await_expander (tree *stmt, int * /*do_subtree*/,
>> void *d)
>> = build1 (ADDR_EXPR, build_reference_type (void_type_node),
>> resume_label);
>>tree susp
>> = build1 (ADDR_EXPR, build_reference_type (void_type_node),
>> data->cororet);
>> + tree cont
>> += build1 (ADDR_EXPR, build_reference_type (void_type_node),
>> data->corocont);
>>tree final_susp = build_int_cst (integer_type_node, is_final ? 1 : 0);
>
> Wait, 'void &' is not a thing. What's been happening here? do you mean to
> build pointer_types? 'build_address (data->$FIELD)’
yes, fixed.
>> @@ -2012,6 +2027,15 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>> tree actor, tree fnbody,
>> = create_named_label_with_ctx (loc, "actor.begin", actor);
>>tree actor_frame = build1_loc (loc, INDIRECT_REF, coro_frame_type,
>> actor_fp);
>> + /* Init the continuation with a NULL ptr. */
>> + tree zeroinit = build1 (CONVERT_EXPR, void_coro_handle_type,
>> + integer_zero_node);
>> + tree ci = build2 (INIT_EXPR, void_coro_handle_type, continuation,
>> zeroinit);
>> + ci = build_stmt (loc, DECL_EXPR, continuation);
>
> This appears to be overwriting ci?
thanks, my carelessness :( .. I failed to delete all the code from an earlier
version.
fixed.
>> + /* Now we have the actual call, and we can mark it as a tail. */
>> + CALL_EXPR_TAILCALL (resume) = true;
>> + /* ... and for optimisation levels 0..1, mark it as requiring a tail-call
>> + for correctness. It seems that doing this for optimisation levels that
>> + normally perform tail-calling, confuses the ME (or it would be logical
>> + to put this on unilaterally). */
>
> Might be worth ping a ME expert about why that is?
David, any comments here?
(confuses == ICE in expand, with an unexpected dead edge, IIRC).
>> + if (optimize < 2)
>> +CALL_EXPR_MUST_TAIL_CALL (resume) = true;
>> + resume = coro_build_cvt_void_expr_stmt (resume, loc);
>> + add_stmt (resume);
>> +
>> + r = build_stmt (loc, RETURN_EXPR, NULL);
>> + r = maybe_cleanup_point_expr_void (r);
>
> Shouldn't there be no cleanups? Perhaps assert it didn't add any?
no, you’re right, that call’s a waste of time - hoist with the C petard
again.
removed (and I’d like to do a trivial follow-up to remove two more instances).
==
amended patch:
coroutines: Implement n4849 recommended symmetric transfer.
Although the note in the text [expr.await] / 5.1.1 is not normative,
it is asserted by users that an implementation that is unable to
perform unlimited symmetric transfers is not terribly useful.
This relates to the following circumstance:
try {
users-function-body:
{
{ some suspend context
continuation_handle = await_suspend (another handle);
continuation_handle.resume ();
'return' (actually a suspension operation).
}
}
} catch (...) {}
The call to 'continuation_handle.resume ()' needs to be a tail-
call in order that an arbitrary number of coroutines can be handled
in this manner. There are two issues with this:
1. That the user's function body is wrapped in a try/catch block and
one cannot tail-call from within those.
2. That GCC doesn't usually produce tail-calls when the optimisation
level is < O2.
After considerable discussion at WG21 meetings, it has been determined
that the intent is that the operation behaves as if the resume call is
executed in the context of the caller.
So, we can remap the fragment above like this:
{
void_coroutine_handle continuation;
try {
users-function-body:
{
Re: [PATCH] coroutines: Implement n4849 recommended symmetric transfer.
On 3/20/20 11:40 AM, Iain Sandoe via Gcc-patches wrote:
2020-03-20 Iain Sandoe
* coroutines.cc (coro_init_identifiers): Initialize an identifier
for the cororoutine handle 'address' method name.
(struct coro_aw_data): Add fields to cover the continuations.
(co_await_expander): Determine the kind of await_suspend in use.
If we have the case that returns a continuation handle, then save
this and make the target for 'scope exit without cleanup' be the
continuation resume label.
(expand_co_awaits): Handle the continuation case.
(struct suspend_point_info): Remove fields that kept the returned
await_suspend handle type.
(transform_await_expr): Remove code tracking continuation handles.
(build_actor_fn): Add the continuation handle as an actor-function
scope var. Build the symmetric transfer continuation point.
(register_await_info): Remove fields tracking continuation handles.
(get_await_suspend_return_type): Remove.
(register_awaits): Remove code tracking continuation handles.
(morph_fn_to_coro): Remove code tracking continuation handles.
gcc/testsuite/ChangeLog:
2020-03-20 Iain Sandoe
* g++.dg/coroutines/torture/co-ret-09-bool-await-susp.C: Amend
to n4849 behaviour.
* g++.dg/coroutines/torture/symmetric-transfer-00-basic.C: New
test.
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index abd4cac1c82..49e03f2ccea 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
tree suspend = TREE_VEC_ELT (awaiter_calls, 1); /* await_suspend(). */
+ tree susp_type;
+ if (TREE_CODE (suspend) == CALL_EXPR)
+{
+ susp_type = CALL_EXPR_FN (suspend);
+ if (TREE_CODE (susp_type) == ADDR_EXPR)
+ susp_type = TREE_OPERAND (susp_type, 0);
+ susp_type = TREE_TYPE (TREE_TYPE (susp_type));
+}
+ else
+susp_type = TREE_TYPE (suspend);
I think:
if (tree fndec = get_callee_fndecl_nofold (suspend))
susp_type = TREE_TYPE (TREE_TYPE (fndecl));
else
susp_type = TREE_TYPE (suspend);
would do? But how can TREE_TYPE (suspend) be different from the return
type of the function decl? It seems funky that the behaviour could
depend on the form of the suspend. What am I missing?
@@ -1532,6 +1553,8 @@ co_await_expander (tree *stmt, int * /*do_subtree*/, void
*d)
= build1 (ADDR_EXPR, build_reference_type (void_type_node), resume_label);
tree susp
= build1 (ADDR_EXPR, build_reference_type (void_type_node),
data->cororet);
+ tree cont
+= build1 (ADDR_EXPR, build_reference_type (void_type_node),
data->corocont);
tree final_susp = build_int_cst (integer_type_node, is_final ? 1 : 0);
Wait, 'void &' is not a thing. What's been happening here? do you mean
to build pointer_types? 'build_address (data->$FIELD)'
@@ -2012,6 +2027,15 @@ build_actor_fn (location_t loc, tree coro_frame_type,
tree actor, tree fnbody,
= create_named_label_with_ctx (loc, "actor.begin", actor);
tree actor_frame = build1_loc (loc, INDIRECT_REF, coro_frame_type,
actor_fp);
+ /* Init the continuation with a NULL ptr. */
+ tree zeroinit = build1 (CONVERT_EXPR, void_coro_handle_type,
+ integer_zero_node);
+ tree ci = build2 (INIT_EXPR, void_coro_handle_type, continuation, zeroinit);
+ ci = build_stmt (loc, DECL_EXPR, continuation);
This appears to be overwriting ci?
+ ci = build1_loc (loc, CONVERT_EXPR, void_type_node, ci);
.. and still expecting it to be an EXPR node?
+ ci = maybe_cleanup_point_expr_void (ci);
+ add_stmt (ci);
+
/* Re-write param references in the body, no code should be generated
here. */
if (DECL_ARGUMENTS (orig))
+
+ /* Now we have the actual call, and we can mark it as a tail. */
+ CALL_EXPR_TAILCALL (resume) = true;
+ /* ... and for optimisation levels 0..1, mark it as requiring a tail-call
+ for correctness. It seems that doing this for optimisation levels that
+ normally perform tail-calling, confuses the ME (or it would be logical
+ to put this on unilaterally). */
Might be worth ping a ME expert about why that is?
+ if (optimize < 2)
+CALL_EXPR_MUST_TAIL_CALL (resume) = true;
+ resume = coro_build_cvt_void_expr_stmt (resume, loc);
+ add_stmt (resume);
+
+ r = build_stmt (loc, RETURN_EXPR, NULL);
+ r = maybe_cleanup_point_expr_void (r);
Shouldn't there be no cleanups? Perhaps assert it didn't add any?
+ add_stmt (r);
+
/* We need the resume index to work with. */
tree res_idx_m
= lookup_member (coro_frame_type, resume_idx_name,
nathan
--
Nathan Sidwell
