coroutines: Amend parameter handling to match n4849.

This is the second in the series to bring the GCC implementation into line
with the current standard.

@JunMa 
  I believe that this should solve the problems you were looking at in 
 “[PATCH Coroutines] Fix issue with unused corutine function parameters”
 and supercedes that patch (since we needed to alter the ordering as well).

  If any parameter issues remain, please file a PR (or a patch, of course).

OK for trunk?
thanks
Iain


=====

In n4849 and preceding versions, [class.copy.elision] (1.3)
appears to confer additional permissions on coroutines to elide
parameter copies.

After considerable discussion on this topic by email and during
the February 2020 WG21 meeting, it has been determined that there
are no additional permissions applicable to coroutine parameter
copy elision.

The content of that clause in the standard is expected to be amended
eventually to clarify this.  Other than this, the handling of
parameter lifetimes is expected to be as per n4849:

 * A copy is made before the promise is constructed
 * If the promise CTOR uses the parms, then it should use the copy
   where appropriate.
 * The param copy lifetimes end after the promise is destroyed
   (during the coroutine frame destruction).
 * Otherwise, C++20 copy elision rules apply.

(as an aside) In practice, we expect that copy elision can only occur
when the coroutine body is fully inlined, possibly in conjunction with
heap allocation elision.

The patch:
 * Reorders the copying process to precede the promise CTOR and
    ensures the correct use.
 * Copies all params into the frame regardless of whether the coro
   body uses them (this is a bit unfortunate, and we should figure
   out an amendment for C++23).

gcc/cp/ChangeLog:

2020-02-26  Iain Sandoe  <iain@sandoe.co.uk>

	* coroutines.cc (struct param_info): Keep track of params
	that are references, and cache the original type and whether
	the DTOR is trivial.
	(build_actor_fn): Handle param copies always, and adjust the
	handling for references.
	(register_param_uses): Only handle uses here.
	(classtype_has_non_deleted_copy_ctor): New.
	(morph_fn_to_coro): Adjust param copy handling to match n4849
	by reordering ahead of the promise CTOR and always making a
	frame copy, even if the param is unused in the coroutine body.

gcc/testsuite/ChangeLog:

2020-02-26  Iain Sandoe  <iain@sandoe.co.uk>

	* g++.dg/coroutines/coro1-refs-and-ctors.h: New.
	* g++.dg/coroutines/torture/func-params-07.C: New test.
	* g++.dg/coroutines/torture/func-params-08.C: New test.
---
 gcc/cp/coroutines.cc                          | 313 +++++++++++-------
 .../g++.dg/coroutines/coro1-refs-and-ctors.h  | 144 ++++++++
 .../coroutines/torture/func-params-07.C       |  81 +++++
 .../coroutines/torture/func-params-08.C       | 112 +++++++
 4 files changed, 524 insertions(+), 126 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/coro1-refs-and-ctors.h
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/func-params-07.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C

On 2/26/20 8:43 AM, Iain Sandoe wrote:
> This is the second in the series to bring the GCC implementation into line
> with the current standard.

> 
> 2020-02-26  Iain Sandoe  <iain@sandoe.co.uk>
> 
> 	* coroutines.cc (struct param_info): Keep track of params
> 	that are references, and cache the original type and whether
> 	the DTOR is trivial.
> 	(build_actor_fn): Handle param copies always, and adjust the
> 	handling for references.
> 	(register_param_uses): Only handle uses here.
> 	(classtype_has_non_deleted_copy_ctor): New.
> 	(morph_fn_to_coro): Adjust param copy handling to match n4849
> 	by reordering ahead of the promise CTOR and always making a
> 	frame copy, even if the param is unused in the coroutine body.

Ok, modulo a couple of changes:

> @@ -1742,6 +1742,11 @@ struct param_info
>     tree field_id;
>     vec<tree *> *body_uses;
>     tree frame_type;
> +  tree orig_type;
> +  bool by_ref;
> +  bool rv_ref;
> +  bool pt_ref;
> +  bool trivial_dtor;
>   };

Can you comment the fields here?

> +bool
> +classtype_has_non_deleted_copy_ctor (tree t)
> +{
> +  if (CLASSTYPE_LAZY_COPY_CTOR (t))
> +    lazily_declare_fn (sfk_copy_constructor, t);
> +  for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (t)); iter; ++iter)
> +    if (copy_fn_p (*iter) && !DECL_DELETED_FN (*iter))
> +      return true;
> +  return false;
> +}

Please put this in class.c next to the non deleted move ctor variant. 
Needs a comment.

Ok with those changes, no need to re-review.

nathan

在 2020/2/26 下午9:43, Iain Sandoe 写道:
> This is the second in the series to bring the GCC implementation into line
> with the current standard.
>
> @JunMa
>    I believe that this should solve the problems you were looking at in
>   “[PATCH Coroutines] Fix issue with unused corutine function parameters”
>   and supercedes that patch (since we needed to alter the ordering as well).
>
>    If any parameter issues remain, please file a PR (or a patch, of course).
>
> OK for trunk?
> thanks
> Iain
>
>
> =====
>
> In n4849 and preceding versions, [class.copy.elision] (1.3)
> appears to confer additional permissions on coroutines to elide
> parameter copies.
>
> After considerable discussion on this topic by email and during
> the February 2020 WG21 meeting, it has been determined that there
> are no additional permissions applicable to coroutine parameter
> copy elision.
>
> The content of that clause in the standard is expected to be amended
> eventually to clarify this.  Other than this, the handling of
> parameter lifetimes is expected to be as per n4849:
>
>   * A copy is made before the promise is constructed
>   * If the promise CTOR uses the parms, then it should use the copy
>     where appropriate.
>   * The param copy lifetimes end after the promise is destroyed
>     (during the coroutine frame destruction).
>   * Otherwise, C++20 copy elision rules apply.
>
> (as an aside) In practice, we expect that copy elision can only occur
> when the coroutine body is fully inlined, possibly in conjunction with
> heap allocation elision.
>
> The patch:
>   * Reorders the copying process to precede the promise CTOR and
>      ensures the correct use.
>   * Copies all params into the frame regardless of whether the coro
>     body uses them (this is a bit unfortunate, and we should figure
>     out an amendment for C++23).
Hi iain，
     This makes sense to me.

Regards
JunMa
> gcc/cp/ChangeLog:
>
> 2020-02-26  Iain Sandoe  <iain@sandoe.co.uk>
>
> 	* coroutines.cc (struct param_info): Keep track of params
> 	that are references, and cache the original type and whether
> 	the DTOR is trivial.
> 	(build_actor_fn): Handle param copies always, and adjust the
> 	handling for references.
> 	(register_param_uses): Only handle uses here.
> 	(classtype_has_non_deleted_copy_ctor): New.
> 	(morph_fn_to_coro): Adjust param copy handling to match n4849
> 	by reordering ahead of the promise CTOR and always making a
> 	frame copy, even if the param is unused in the coroutine body.
>
> gcc/testsuite/ChangeLog:
>
> 2020-02-26  Iain Sandoe  <iain@sandoe.co.uk>
>
> 	* g++.dg/coroutines/coro1-refs-and-ctors.h: New.
> 	* g++.dg/coroutines/torture/func-params-07.C: New test.
> 	* g++.dg/coroutines/torture/func-params-08.C: New test.
> ---
>   gcc/cp/coroutines.cc                          | 313 +++++++++++-------
>   .../g++.dg/coroutines/coro1-refs-and-ctors.h  | 144 ++++++++
>   .../coroutines/torture/func-params-07.C       |  81 +++++
>   .../coroutines/torture/func-params-08.C       | 112 +++++++
>   4 files changed, 524 insertions(+), 126 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/coro1-refs-and-ctors.h
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/func-params-07.C
>   create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C
>
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 524d4872804..e0e7e66fe5e 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -1742,6 +1742,11 @@ struct param_info
>     tree field_id;
>     vec<tree *> *body_uses;
>     tree frame_type;
> +  tree orig_type;
> +  bool by_ref;
> +  bool rv_ref;
> +  bool pt_ref;
> +  bool trivial_dtor;
>   };
>   
>   struct local_var_info
> @@ -1941,26 +1946,37 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody,
>   
>     /* Re-write param references in the body, no code should be generated
>        here.  */
> -  if (DECL_ARGUMENTS (orig) && param_uses != NULL)
> +  if (DECL_ARGUMENTS (orig))
>       {
>         tree arg;
>         for (arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg))
>   	{
>   	  bool existed;
>   	  param_info &parm = param_uses->get_or_insert (arg, &existed);
> -	  if (parm.field_id == NULL_TREE)
> -	    continue; /* Wasn't used.  */
> +	  if (!parm.body_uses)
> +	    continue; /* Wasn't used in the orignal function body.  */
> +
>   	  tree fld_ref = lookup_member (coro_frame_type, parm.field_id,
>   					/*protect=*/1, /*want_type=*/0,
>   					tf_warning_or_error);
> -	  tree fld_idx = build3_loc (loc, COMPONENT_REF, TREE_TYPE (arg),
> +	  tree fld_idx = build3_loc (loc, COMPONENT_REF, parm.frame_type,
>   				     actor_frame, fld_ref, NULL_TREE);
> +
> +	  /* We keep these in the frame as a regular pointer, so convert that
> +	   back to the type expected.  */
> +	  if (parm.pt_ref)
> +	    fld_idx = build1_loc (loc, CONVERT_EXPR, TREE_TYPE (arg), fld_idx);
> +
> +	  /* We expect an rvalue ref. here.  */
> +	  if (parm.rv_ref)
> +	    fld_idx = convert_to_reference (DECL_ARG_TYPE (arg), fld_idx,
> +					    CONV_STATIC, LOOKUP_NORMAL,
> +					    NULL_TREE, tf_warning_or_error);
> +
>   	  int i;
>   	  tree *puse;
>   	  FOR_EACH_VEC_ELT (*parm.body_uses, i, puse)
> -	    {
> -	      *puse = fld_idx;
> -	    }
> +	    *puse = fld_idx;
>   	}
>       }
>   
> @@ -2837,27 +2853,8 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d)
>     param_info &parm = data->param_uses->get_or_insert (*stmt, &existed);
>     gcc_checking_assert (existed);
>   
> -  if (parm.field_id == NULL_TREE)
> +  if (!parm.body_uses)
>       {
> -      tree actual_type = TREE_TYPE (*stmt);
> -
> -      if (!COMPLETE_TYPE_P (actual_type))
> -	actual_type = complete_type_or_else (actual_type, *stmt);
> -
> -      if (actual_type == NULL_TREE)
> -	/* Diagnostic emitted by complete_type_or_else.  */
> -	actual_type = error_mark_node;
> -
> -      if (TREE_CODE (actual_type) == REFERENCE_TYPE)
> -	actual_type = build_pointer_type (TREE_TYPE (actual_type));
> -
> -      parm.frame_type = actual_type;
> -      tree pname = DECL_NAME (*stmt);
> -      size_t namsize = sizeof ("__parm.") + IDENTIFIER_LENGTH (pname) + 1;
> -      char *buf = (char *) alloca (namsize);
> -      snprintf (buf, namsize, "__parm.%s", IDENTIFIER_POINTER (pname));
> -      parm.field_id
> -	= coro_make_frame_entry (data->field_list, buf, actual_type, data->loc);
>         vec_alloc (parm.body_uses, 4);
>         parm.body_uses->quick_push (stmt);
>         data->param_seen = true;
> @@ -2970,6 +2967,17 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name)
>     return fn;
>   }
>   
> +bool
> +classtype_has_non_deleted_copy_ctor (tree t)
> +{
> +  if (CLASSTYPE_LAZY_COPY_CTOR (t))
> +    lazily_declare_fn (sfk_copy_constructor, t);
> +  for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (t)); iter; ++iter)
> +    if (copy_fn_p (*iter) && !DECL_DELETED_FN (*iter))
> +      return true;
> +  return false;
> +}
> +
>   /* Here we:
>      a) Check that the function and promise type are valid for a
>         coroutine.
> @@ -2993,11 +3001,13 @@ act_des_fn (tree orig, tree fn_type, tree coro_frame_ptr, const char* name)
>     coro1::promise_type __p;
>     bool frame_needs_free; free the coro frame mem if set.
>     short __resume_at;
> +  handle_type self_handle;
> +  (maybe) parameter copies.
> +  (maybe) lambda capture copies.
>     coro1::suspend_never_prt __is;
>     (maybe) handle_type i_hand;
>     coro1::suspend_always_prt __fs;
>     (maybe) handle_type f_hand;
> -  (maybe) parameters used in the body.
>     (maybe) local variables saved
>     (maybe) trailing space.
>    };  */
> @@ -3120,6 +3130,77 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>        await_suspend().  There's no point in creating it over and over.  */
>     (void) coro_make_frame_entry (&field_list, "__self_h", handle_type, fn_start);
>   
> +  /* Now add in fields for function params (if there are any).
> +     We do not attempt elision of copies at this stage, we do analyse the
> +     uses and build worklists to replace those when the state machine is
> +     lowered.  */
> +
> +  hash_map<tree, param_info> *param_uses = NULL;
> +  if (DECL_ARGUMENTS (orig))
> +    {
> +      /* Build a hash map with an entry for each param.
> +	  The key is the param tree.
> +	  Then we have an entry for the frame field name.
> +	  Then a cache for the field ref when we come to use it.
> +	  Then a tree list of the uses.
> +	  The second two entries start out empty - and only get populated
> +	  when we see uses.  */
> +      param_uses = new hash_map<tree, param_info>;
> +
> +      for (tree arg = DECL_ARGUMENTS (orig); arg != NULL;
> +	   arg = DECL_CHAIN (arg))
> +	{
> +	  bool existed;
> +	  param_info &parm = param_uses->get_or_insert (arg, &existed);
> +	  gcc_checking_assert (!existed);
> +	  parm.body_uses = NULL;
> +	  tree actual_type = TREE_TYPE (arg);
> +	  actual_type = complete_type_or_else (actual_type, orig);
> +	  if (actual_type == NULL_TREE)
> +	    actual_type = error_mark_node;
> +	  parm.orig_type = actual_type;
> +	  parm.by_ref = parm.rv_ref = parm.pt_ref = false;
> +	  if (TREE_CODE (actual_type) == REFERENCE_TYPE
> +	      && TYPE_REF_IS_RVALUE (DECL_ARG_TYPE (arg)))
> +	    {
> +	      parm.rv_ref = true;
> +	      actual_type = TREE_TYPE (actual_type);
> +	      parm.frame_type = actual_type;
> +	    }
> +	  else if (TREE_CODE (actual_type) == REFERENCE_TYPE)
> +	    {
> +	      /* If the user passes by reference, then we will save the
> +		 pointer to the original.  As noted in
> +		 [dcl.fct.def.coroutine] / 13, if the lifetime of the
> +		 referenced item ends and then the coroutine is resumed,
> +		 we have UB; well, the user asked for it.  */
> +	      actual_type = build_pointer_type (TREE_TYPE (actual_type));
> +	      parm.frame_type = actual_type;
> +	      parm.pt_ref = true;
> +	    }
> +	  else if (TYPE_REF_P (DECL_ARG_TYPE (arg)))
> +	    {
> +	      parm.by_ref = true;
> +	      parm.frame_type = actual_type;
> +	    }
> +	  else
> +	    parm.frame_type = actual_type;
> +
> +	  parm.trivial_dtor = TYPE_HAS_TRIVIAL_DESTRUCTOR (parm.frame_type);
> +	  tree pname = DECL_NAME (arg);
> +	  char *buf = xasprintf ("__parm.%s", IDENTIFIER_POINTER (pname));
> +	  parm.field_id = coro_make_frame_entry
> +	    (&field_list, buf, actual_type, DECL_SOURCE_LOCATION (arg));
> +	  free (buf);
> +	}
> +
> +      param_frame_data param_data
> +	= {&field_list, param_uses, fn_start, false};
> +      /* We want to record every instance of param's use, so don't include
> +	 a 'visited' hash_set.  */
> +      cp_walk_tree (&fnbody, register_param_uses, &param_data, NULL);
> +    }
> +
>     /* Initial suspend is mandated.  */
>     tree init_susp_name = coro_make_frame_entry (&field_list, "__aw_s.is",
>   					       initial_suspend_type, fn_start);
> @@ -3164,50 +3245,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>     register_await_info (final_await, final_suspend_type, fin_susp_name,
>   		       void_type_node, fin_hand_name);
>   
> -  /* 3. Now add in fields for function params (if there are any) that are used
> -     within the function body.  This is conservative; we can't tell at this
> -     stage if such uses might be optimized away, or if they might turn out not
> -     to persist across any suspend points.  Of course, even if they don't
> -     persist across suspend points, when the actor is out of line the saved
> -     frame version is still needed.  */
> -  hash_map<tree, param_info> *param_uses = NULL;
> -  if (DECL_ARGUMENTS (orig))
> -    {
> -      /* Build a hash map with an entry for each param.
> -	  The key is the param tree.
> -	  Then we have an entry for the frame field name.
> -	  Then a cache for the field ref when we come to use it.
> -	  Then a tree list of the uses.
> -	  The second two entries start out empty - and only get populated
> -	  when we see uses.  */
> -      param_uses = new hash_map<tree, param_info>;
> -
> -      for (tree arg = DECL_ARGUMENTS (orig); arg != NULL;
> -	   arg = DECL_CHAIN (arg))
> -	{
> -	  bool existed;
> -	  param_info &parm = param_uses->get_or_insert (arg, &existed);
> -	  gcc_checking_assert (!existed);
> -	  parm.field_id = NULL_TREE;
> -	  parm.body_uses = NULL;
> -	}
> -
> -      param_frame_data param_data
> -	= {&field_list, param_uses, fn_start, false};
> -      /* We want to record every instance of param's use, so don't include
> -	 a 'visited' hash_set.  */
> -      cp_walk_tree (&fnbody, register_param_uses, &param_data, NULL);
> -
> -      /* If no uses for any param were seen, act as if there were no
> -	 params (it could be that they are only used to construct the
> -	 promise).  */
> -      if (!param_data.param_seen)
> -	{
> -	  delete param_uses;
> -	  param_uses = NULL;
> -	}
> -    }
> -
>     /* 4. Now make space for local vars, this is conservative again, and we
>        would expect to delete unused entries later.  */
>     hash_map<tree, local_var_info> local_var_uses;
> @@ -3480,59 +3517,34 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>     r = coro_build_cvt_void_expr_stmt (r, fn_start);
>     add_stmt (r);
>   
> -  /* Set up the promise.  */
> -  tree promise_m
> -    = lookup_member (coro_frame_type, promise_name,
> -		     /*protect=*/1, /*want_type=*/0, tf_warning_or_error);
> +  /* n4849 [dcl.fct.def.coroutine] /13
> +     When a coroutine is invoked, a copy is created for each coroutine
> +     parameter.  Each such copy is an object with automatic storage duration
> +     that is direct-initialized from an lvalue referring to the corresponding
> +     parameter if the parameter is an lvalue reference, and from an xvalue
> +     referring to it otherwise.  A reference to a parameter in the function-
> +     body of the coroutine and in the call to the coroutine promise
> +     constructor is replaced by a reference to its copy.  */
>   
> -  tree p = build_class_member_access_expr (deref_fp, promise_m, NULL_TREE,
> -					   false, tf_warning_or_error);
> +  vec<tree, va_gc> *promise_args = NULL; /* So that we can adjust refs.  */
>   
> -  if (TYPE_NEEDS_CONSTRUCTING (promise_type))
> -    {
> -      /* Do a placement new constructor for the promise type (we never call
> -	 the new operator, just the constructor on the object in place in the
> -	 frame).
> +  /* The initialization and destruction of each parameter copy occurs in the
> +     context of the called coroutine.  Initializations of parameter copies are
> +     sequenced before the call to the coroutine promise constructor and
> +     indeterminately sequenced with respect to each other.  The lifetime of
> +     parameter copies ends immediately after the lifetime of the coroutine
> +     promise object ends.  */
>   
> -	 First try to find a constructor with the same parameter list as the
> -	 original function (if it has params), failing that find a constructor
> -	 with no parameter list.  */
> -
> -      if (DECL_ARGUMENTS (orig))
> -	{
> -	  vec<tree, va_gc> *args = make_tree_vector ();
> -	  tree arg;
> -	  for (arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg))
> -	    vec_safe_push (args, arg);
> -	  r = build_special_member_call (p, complete_ctor_identifier, &args,
> -					 promise_type, LOOKUP_NORMAL, tf_none);
> -	  release_tree_vector (args);
> -	}
> -      else
> -	r = NULL_TREE;
> -
> -      if (r == NULL_TREE || r == error_mark_node)
> -	r = build_special_member_call (p, complete_ctor_identifier, NULL,
> -				       promise_type, LOOKUP_NORMAL,
> -				       tf_warning_or_error);
> -
> -      r = coro_build_cvt_void_expr_stmt (r, fn_start);
> -      add_stmt (r);
> -    }
> -
> -  /* Copy in any of the function params we found to be used.
> -     Param types with non-trivial dtors will have to be moved into position
> -     and the dtor run before the frame is freed.  */
>     vec<tree, va_gc> *param_dtor_list = NULL;
> -  if (DECL_ARGUMENTS (orig) && param_uses != NULL)
> +
> +  if (DECL_ARGUMENTS (orig))
>       {
> -      tree arg;
> -      for (arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg))
> +      promise_args = make_tree_vector ();
> +      for (tree arg = DECL_ARGUMENTS (orig); arg != NULL;
> +	   arg = DECL_CHAIN (arg))
>   	{
>   	  bool existed;
>   	  param_info &parm = param_uses->get_or_insert (arg, &existed);
> -	  if (parm.field_id == NULL_TREE)
> -	    continue; /* Wasn't used.  */
>   
>   	  tree fld_ref = lookup_member (coro_frame_type, parm.field_id,
>   					/*protect=*/1, /*want_type=*/0,
> @@ -3541,14 +3553,21 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	    = build_class_member_access_expr (deref_fp, fld_ref, NULL_TREE,
>   					      false, tf_warning_or_error);
>   
> +	  /* Add this to the promise CTOR arguments list, accounting for
> +	     refs.  */
> +	  if (parm.by_ref)
> +	    vec_safe_push (promise_args, fld_idx);
> +	  else if (parm.rv_ref)
> +	    vec_safe_push (promise_args, rvalue (fld_idx));
> +	  else
> +	    vec_safe_push (promise_args, arg);
> +
>   	  if (TYPE_NEEDS_CONSTRUCTING (parm.frame_type))
>   	    {
>   	      vec<tree, va_gc> *p_in;
> -	      if (TYPE_REF_P (DECL_ARG_TYPE (arg))
> -		  && (CLASSTYPE_LAZY_MOVE_CTOR (parm.frame_type)
> -		      || CLASSTYPE_LAZY_MOVE_ASSIGN (parm.frame_type)
> -		      || classtype_has_move_assign_or_move_ctor_p
> -			    (parm.frame_type, /*user_declared=*/true)))
> +	      if (parm.by_ref
> +		  && classtype_has_non_deleted_move_ctor (parm.frame_type)
> +		  && !classtype_has_non_deleted_copy_ctor (parm.frame_type))
>   		p_in = make_tree_vector_single (rvalue (arg));
>   	      else
>   		p_in = make_tree_vector_single (arg);
> @@ -3558,13 +3577,12 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   					     LOOKUP_NORMAL,
>   					     tf_warning_or_error);
>   	      release_tree_vector (p_in);
> -	      if (param_dtor_list == NULL)
> -		param_dtor_list = make_tree_vector ();
> -	      vec_safe_push (param_dtor_list, parm.field_id);
>   	    }
>   	  else
>   	    {
> -	      if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))
> +	      if (parm.rv_ref)
> +		r = convert_from_reference (arg);
> +	      else if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))
>   		r = build1_loc (DECL_SOURCE_LOCATION (arg), CONVERT_EXPR,
>   				parm.frame_type, arg);
>   	      else
> @@ -3575,9 +3593,52 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>   	    }
>   	  r = coro_build_cvt_void_expr_stmt (r, fn_start);
>   	  add_stmt (r);
> +	  if (!parm.trivial_dtor)
> +	    {
> +	      if (param_dtor_list == NULL)
> +		param_dtor_list = make_tree_vector ();
> +	      vec_safe_push (param_dtor_list, parm.field_id);
> +	    }
>   	}
>       }
>   
> +  /* Set up the promise.  */
> +  tree promise_m
> +    = lookup_member (coro_frame_type, promise_name,
> +		     /*protect=*/1, /*want_type=*/0, tf_warning_or_error);
> +
> +  tree p = build_class_member_access_expr (deref_fp, promise_m, NULL_TREE,
> +					   false, tf_warning_or_error);
> +
> +  if (TYPE_NEEDS_CONSTRUCTING (promise_type))
> +    {
> +      /* Do a placement new constructor for the promise type (we never call
> +	 the new operator, just the constructor on the object in place in the
> +	 frame).
> +
> +	 First try to find a constructor with the same parameter list as the
> +	 original function (if it has params), failing that find a constructor
> +	 with no parameter list.  */
> +
> +      if (DECL_ARGUMENTS (orig))
> +	{
> +	  r = build_special_member_call (p, complete_ctor_identifier,
> +					 &promise_args, promise_type,
> +					 LOOKUP_NORMAL, tf_none);
> +	  release_tree_vector (promise_args);
> +	}
> +      else
> +	r = NULL_TREE;
> +
> +      if (r == NULL_TREE || r == error_mark_node)
> +	r = build_special_member_call (p, complete_ctor_identifier, NULL,
> +				       promise_type, LOOKUP_NORMAL,
> +				       tf_warning_or_error);
> +
> +      r = coro_build_cvt_void_expr_stmt (r, fn_start);
> +      add_stmt (r);
> +    }
> +
>     vec<tree, va_gc> *captures_dtor_list = NULL;
>     while (!captures.is_empty())
>       {
> diff --git a/gcc/testsuite/g++.dg/coroutines/coro1-refs-and-ctors.h b/gcc/testsuite/g++.dg/coroutines/coro1-refs-and-ctors.h
> new file mode 100644
> index 00000000000..5b89e7c4128
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/coro1-refs-and-ctors.h
> @@ -0,0 +1,144 @@
> +struct coro1 {
> +
> +  struct promise_type {
> +
> +  promise_type () : vv(-1) {  PRINT ("Promise def. CTOR"); }
> +  promise_type (int __x) : vv(__x) {  PRINTF ("Created Promise with %d\n",__x); }
> +  promise_type (int __x, int& __y, int&& __z)
> +    : vv(__x), v2(__y), v3(__z)
> +    {  PRINTF ("Created Promise with %d, %d, %d\n", __x, __y, __z); }
> +
> +  ~promise_type() { PRINT ("Destroyed Promise"); }
> +
> +  auto get_return_object () {
> +    PRINT ("get_return_object: handle from promise");
> +    return handle_type::from_promise (*this);
> +  }
> +
> +  auto initial_suspend () {
> +    PRINT ("get initial_suspend (always)");
> +    return suspend_always_prt{};
> +  }
> +  auto final_suspend () {
> +    PRINT ("get final_suspend (always)");
> +    return suspend_always_prt{};
> +  }
> +
> +#ifdef USE_AWAIT_TRANSFORM
> +
> +  auto await_transform (int v) {
> +    PRINTF ("await_transform an int () %d\n",v);
> +    return suspend_always_intprt (v);
> +  }
> +
> +  auto await_transform (long v) {
> +    PRINTF ("await_transform a long () %ld\n",v);
> +    return suspend_always_longprtsq (v);
> +  }
> +
> +#endif
> +
> +  auto yield_value (int v) {
> +    PRINTF ("yield_value (%d)\n", v);
> +    vv = v;
> +    return suspend_always_prt{};
> +  }
> +
> +  void return_value (int v) {
> +    PRINTF ("return_value (%d)\n", v);
> +    vv = v;
> +
> +  }
> +
> +  void unhandled_exception() { PRINT ("** unhandled exception"); }
> +
> +  int get_value () { return vv; }
> +  int get_v2 () { return v2; }
> +  int get_v3 () { return v3; }
> +
> +  private:
> +    int vv;
> +    int v2;
> +    int v3;
> +  };
> +
> +  using handle_type = coro::coroutine_handle<coro1::promise_type>;
> +  handle_type handle;
> +  coro1 () : handle(0) {}
> +  coro1 (handle_type _handle)
> +    : handle(_handle) {
> +        PRINT("Created coro1 object from handle");
> +  }
> +  coro1 (const coro1 &) = delete; // no copying
> +  coro1 (coro1 &&s) : handle(s.handle) {
> +	s.handle = nullptr;
> +	PRINT("coro1 mv ctor ");
> +  }
> +  coro1 &operator = (coro1 &&s) {
> +	handle = s.handle;
> +	s.handle = nullptr;
> +	PRINT("coro1 op=  ");
> +	return *this;
> +  }
> +  ~coro1() {
> +        PRINT("Destroyed coro1");
> +        if ( handle )
> +          handle.destroy();
> +  }
> +
> +  // Some awaitables to use in tests.
> +  // With progress printing for debug.
> +  struct suspend_never_prt {
> +  bool await_ready() const noexcept { return true; }
> +  void await_suspend(handle_type) const noexcept { PRINT ("susp-never-susp");}
> +  void await_resume() const noexcept { PRINT ("susp-never-resume");}
> +  };
> +
> +  struct  suspend_always_prt {
> +  bool await_ready() const noexcept { return false; }
> +  void await_suspend(handle_type) const noexcept { PRINT ("susp-always-susp");}
> +  void await_resume() const noexcept { PRINT ("susp-always-resume");}
> +  ~suspend_always_prt() { PRINT ("susp-always-dtor"); }
> +  };
> +
> +  struct suspend_always_intprt {
> +    int x;
> +    suspend_always_intprt() : x(5) {}
> +    suspend_always_intprt(int __x) : x(__x) {}
> +    ~suspend_always_intprt() {}
> +    bool await_ready() const noexcept { return false; }
> +    void await_suspend(coro::coroutine_handle<>) const noexcept { PRINT ("susp-always-susp-intprt");}
> +    int await_resume() const noexcept { PRINT ("susp-always-resume-intprt"); return x;}
> +  };
> +
> +  /* This returns the square of the int that it was constructed with.  */
> +  struct suspend_always_longprtsq {
> +    long x;
> +    suspend_always_longprtsq() : x(12L) { PRINT ("suspend_always_longprtsq def ctor"); }
> +    suspend_always_longprtsq(long _x) : x(_x) { PRINTF ("suspend_always_longprtsq ctor with %ld\n", x); }
> +    ~suspend_always_longprtsq() {}
> +    bool await_ready() const noexcept { return false; }
> +    void await_suspend(coro::coroutine_handle<>) const noexcept { PRINT ("susp-always-susp-longsq");}
> +    long await_resume() const noexcept { PRINT ("susp-always-resume-longsq"); return x * x;}
> +  };
> +
> +  struct suspend_always_intrefprt {
> +    int& x;
> +    suspend_always_intrefprt(int& __x) : x(__x) {}
> +    ~suspend_always_intrefprt() {}
> +    bool await_ready() const noexcept { return false; }
> +    void await_suspend(coro::coroutine_handle<>) const noexcept { PRINT ("susp-always-susp-intprt");}
> +    int& await_resume() const noexcept { PRINT ("susp-always-resume-intprt"); return x;}
> +  };
> +
> +  template <typename _AwaitType>
> +  struct suspend_always_tmpl_awaiter {
> +    _AwaitType x;
> +    suspend_always_tmpl_awaiter(_AwaitType __x) : x(__x) {}
> +    ~suspend_always_tmpl_awaiter() {}
> +    bool await_ready() const noexcept { return false; }
> +    void await_suspend(coro::coroutine_handle<>) const noexcept { PRINT ("suspend_always_tmpl_awaiter");}
> +    _AwaitType await_resume() const noexcept { PRINT ("suspend_always_tmpl_awaiter"); return x;}
> +  };
> +
> +};
> diff --git a/gcc/testsuite/g++.dg/coroutines/torture/func-params-07.C b/gcc/testsuite/g++.dg/coroutines/torture/func-params-07.C
> new file mode 100644
> index 00000000000..7f3bb3cc782
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/torture/func-params-07.C
> @@ -0,0 +1,81 @@
> +// { dg-do run }
> +
> +// Test that we copy simple parms correctly by value, reference or
> +// rvalue reference.
> +
> +#include "../coro.h"
> +
> +// boiler-plate for tests of codegen
> +#include "../coro1-refs-and-ctors.h"
> +
> +coro1
> +my_coro (int v1, int& v2, int&& v3)
> +{
> +  co_yield v1 + v2 + v3;
> +  co_return v1 + v2 + v3;
> +}
> +
> +int main ()
> +{
> +  PRINT ("main: create coro1");
> +  int lv = 1;
> +  int lvr = 2;
> +  coro1 x = my_coro (lv, lvr, lvr+2);
> +
> +  if (x.handle.done())
> +    abort();
> +
> +  x.handle.resume();
> +  PRINT ("main: after resume (initial suspend)");
> +
> +  /* Now we should have the co_yielded value.  */
> +  int y = x.handle.promise().get_value();
> +  if ( y != 7 )
> +    {
> +      PRINTF ("main: wrong result (%d).", y);
> +      abort ();
> +    }
> +
> +  /* So we should be suspended at the yield, now change the
> +     values so that we can determine that the reference persists
> +     and the copy was made correctly.  */
> +  lv = 5; // should be ignored
> +  lvr = 3; // should be enacted
> +
> +  x.handle.resume();
> +  PRINT ("main: after resume (yield)");
> +
> +  /* Now we should have the co_returned value.  */
> +  y = x.handle.promise().get_value();
> +  if ( y != 8 )
> +    {
> +      PRINTF ("main: wrong result (%d).", y);
> +      abort ();
> +    }
> +
> +  y = x.handle.promise().get_v2();
> +  if ( y != 2 )
> +    {
> +      PRINTF ("main: wrong result 2 (%d).", y);
> +      abort ();
> +    }
> +
> +  y = x.handle.promise().get_v3();
> +  if ( y != 4 )
> +    {
> +      PRINTF ("main: wrong result 3 (%d).", y);
> +      abort ();
> +    }
> +
> +  if (!x.handle.done())
> +    {
> +      PRINT ("main: apparently not done...");
> +      abort ();
> +    }
> +
> +  x.handle.destroy();
> +  x.handle = NULL;
> +
> +  PRINT ("main: returning");
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C b/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C
> new file mode 100644
> index 00000000000..c34d143fa68
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/coroutines/torture/func-params-08.C
> @@ -0,0 +1,112 @@
> +// { dg-do run }
> +
> +// Check that we correctly handle params with non-trivial DTORs and
> +// use the correct copy/move CTORs.
> +
> +#include "../coro.h"
> +#include <vector>
> +
> +// boiler-plate for tests of codegen
> +#include "../coro1-ret-int-yield-int.h"
> +
> +int regular = 0;
> +int copy = 0;
> +int move = 0;
> +
> +struct Foo {
> +  Foo(int _v) : value(_v), x(1, _v)
> +    {
> +      regular++;
> +      PRINTF ("FOO(%d)\n",_v);
> +    }
> +
> +  Foo(const Foo& t)
> +    {
> +      value = t.value;
> +      x = t.x;
> +      copy++;
> +      PRINTF ("FOO(&), %d\n",value);
> +    }
> +
> +  Foo(Foo&& s)
> +    {
> +      value = s.value;
> +      s.value = -1;
> +      x = std::move(s.x);
> +      s.x = std::vector<int> ();
> +      move++;
> +      PRINTF ("FOO(&&), %d\n", value);
> +    }
> +
> +  ~Foo() {PRINTF ("~FOO(), %d\n", value);}
> +
> +  auto operator co_await()
> +    {
> +      struct awaitable
> +	{
> +	  int v;
> +	  awaitable (int _v) : v(_v) {}
> +	  bool await_ready() { return true; }
> +	  void await_suspend(coro::coroutine_handle<>) {}
> +	  int await_resume() { return v;}
> +	};
> +      return awaitable{value + x[0]};
> +    };
> +
> +    void return_value(int _v) { value = _v;}
> +
> +    int value;
> +    std::vector<int> x;
> +};
> +
> +coro1
> +my_coro (Foo t_lv, Foo& t_ref, Foo&& t_rv_ref)
> +{
> +  PRINT ("my_coro");
> +  // We are created suspended, so correct operation depends on
> +  // the parms being copied.
> +  int sum = co_await t_lv;
> +  PRINT ("my_coro 1");
> +  sum += co_await t_ref;
> +  PRINT ("my_coro 2");
> +  sum += co_await t_rv_ref;
> +  PRINT ("my_coro 3");
> +  co_return sum;
> +}
> +
> +int main ()
> +{
> +
> +  PRINT ("main: create coro1");
> +  Foo thing (4);
> +  coro1 x = my_coro (Foo (1), thing, Foo (2));
> +  PRINT ("main: done ramp");
> +
> +  if (x.handle.done())
> +    abort();
> +  x.handle.resume();
> +  PRINT ("main: after resume (initial suspend)");
> +
> +  // now do the three co_awaits.
> +  while(!x.handle.done())
> +    x.handle.resume();
> +  PRINT ("main: after resuming 3 co_awaits");
> +
> +  /* Now we should have the co_returned value.  */
> +  int y = x.handle.promise().get_value();
> +  if (y != 14)
> +    {
> +      PRINTF ("main: wrong result (%d).", y);
> +      abort ();
> +    }
> +
> +  if (regular != 3 || copy != 1 || move != 1)
> +    {
> +      PRINTF ("main: unexpected ctor use (R:%d, C:%d, M:%d)\n",
> +	      regular, copy, move);
> +      abort ();
> +    }
> +
> +  PRINT ("main: returning");
> +  return 0;
> +}