[PATCH] coroutines: Implement n4849 recommended symmetric transfer.

[Nathan Sidwell]

On 3/20/20 11:40 AM, Iain Sandoe via Gcc-patches wrote:

> 2020-03-20  Iain Sandoe  <iain@sandoe.co.uk>
> 
> 	* 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  <iain@sandoe.co.uk>
> 
> 	* 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