[coroutines] Change lowering behavior of alias variable from copy to substitute

Hi
When testing coroutines with lambda function, I find we copy each captured
variable to frame. However, according to gimplify pass, for each declaration
that is an alias for another expression(DECL_VALUE_EXPR), we can
substitute them directly.

Since lambda captured variables is one of this kind. It is better to 
replace them
rather than copy them, This can reduce frame size (all of the captured 
variables
are field of closure class) and avoid extra copy behavior as well.

This patch remove all of the code related to copy captured variable.
Instead, we first rewrite DECL_VALUE_EXPR with frame field, then we check
every variable whether it has DECL_VALUE_EXPR, and then substitute it, this
patch does not create frame field for such variables.

Bootstrap and test on X86_64, is it OK?

Regards
JunMa

gcc/cp
2020-02-04  Jun Ma <JunMa@linux.alibaba.com>

         * coroutines.cc (morph_fn_to_coro): Remove code related to
         copy captured variable.
         (register_local_var_uses):  Ditto.
         (register_param_uses):  Collect use of parameters inside
         DECL_VALUE_EXPR.
         (transform_local_var_uses): Substitute the alias variable
         with DECL_VALUE_EXPR if it has one.


gcc/testsuite
2020-02-04  Jun Ma <JunMa@linux.alibaba.com>

         * g++.dg/coroutines/lambda-07-multi-capture.C: New test.
---
 gcc/cp/coroutines.cc                          | 117 +++++-------------
 .../torture/lambda-07-multi-capture.C         |  55 ++++++++
 2 files changed, 85 insertions(+), 87 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C

在 2020/2/4 下午8:17, JunMa 写道:
> Hi
> When testing coroutines with lambda function, I find we copy each 
> captured
> variable to frame. However, according to gimplify pass, for each 
> declaration
> that is an alias for another expression(DECL_VALUE_EXPR), we can
> substitute them directly.
>
> Since lambda captured variables is one of this kind. It is better to 
> replace them
> rather than copy them, This can reduce frame size (all of the captured 
> variables
> are field of closure class) and avoid extra copy behavior as well.
>
> This patch remove all of the code related to copy captured variable.
> Instead, we first rewrite DECL_VALUE_EXPR with frame field, then we check
> every variable whether it has DECL_VALUE_EXPR, and then substitute it, 
> this
> patch does not create frame field for such variables.
>
> Bootstrap and test on X86_64, is it OK?
>
> Regards
> JunMa
>
Hi

minor update: only handle var_decl when iterate BIND_EXPR_VARS
in register_local_var_uses.

Regards
JunMa
> gcc/cp
> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>
>         * coroutines.cc (morph_fn_to_coro): Remove code related to
>         copy captured variable.
>         (register_local_var_uses):  Ditto.
>         (register_param_uses):  Collect use of parameters inside
>         DECL_VALUE_EXPR.
>         (transform_local_var_uses): Substitute the alias variable
>         with DECL_VALUE_EXPR if it has one.
>
>
> gcc/testsuite
> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>
>         * g++.dg/coroutines/lambda-07-multi-capture.C: New test.
---
 gcc/cp/coroutines.cc                          | 117 +++++-------------
 .../torture/lambda-07-multi-capture.C         |  55 ++++++++
 2 files changed, 85 insertions(+), 87 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 0c2ec32d7db..1bc2ed7f89c 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1790,6 +1790,11 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  cp_walk_tree (&DECL_SIZE (lvar), transform_local_var_uses, d, NULL);
 	  cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d,
 			NULL);
+	  if (VAR_P (lvar) && DECL_HAS_VALUE_EXPR_P (lvar))
+	    {
+	      tree t  = DECL_VALUE_EXPR (lvar);
+	      cp_walk_tree (&t, transform_local_var_uses, d, NULL);
+	    }
 
 	  /* TODO: implement selective generation of fields when vars are
 	     known not-used.  */
@@ -1815,9 +1820,9 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  gcc_checking_assert (existed);
 
 	  if (local_var.field_id == NULL_TREE)
-	    pvar = &DECL_CHAIN (*pvar); /* Wasn't used.  */
-
-	  *pvar = DECL_CHAIN (*pvar); /* discard this one, we replaced it.  */
+	    pvar = &DECL_CHAIN (*pvar); /* Wasn't used or was an alias.  */
+	  else
+	    *pvar = DECL_CHAIN (*pvar); /* discard this one, we replaced it.  */
 	}
 
       *do_subtree = 0; /* We've done the body already.  */
@@ -1847,8 +1852,16 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
   if (local_var_i == NULL)
     return NULL_TREE;
 
-  /* This is our revised 'local' i.e. a frame slot.  */
-  tree revised = local_var_i->field_idx;
+  /* This is our revised 'local' i.e. a frame slot or an alias.  */
+  tree revised  = NULL_TREE;
+  if (local_var_i->field_id == NULL_TREE)
+    {
+      gcc_checking_assert (DECL_HAS_VALUE_EXPR_P (var_decl));
+      /* If the decl is an alias for another expression, substitute it.  */
+      revised = DECL_VALUE_EXPR (var_decl);
+    }
+  else
+    revised = local_var_i->field_idx;
   gcc_checking_assert (DECL_CONTEXT (var_decl) == lvd->context);
 
   if (decl_expr_p && DECL_INITIAL (var_decl))
@@ -2796,6 +2809,12 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d)
 {
   param_frame_data *data = (param_frame_data *) d;
 
+  if (VAR_P (*stmt) && DECL_HAS_VALUE_EXPR_P (*stmt))
+    {
+      tree x = DECL_VALUE_EXPR (*stmt);
+      cp_walk_tree (&x, register_param_uses, d, NULL);
+    }
+
   if (TREE_CODE (*stmt) != PARM_DECL)
     return NULL_TREE;
 
@@ -2840,7 +2859,6 @@ struct local_vars_frame_data
 {
   tree *field_list;
   hash_map<tree, local_var_info> *local_var_uses;
-  vec<local_var_info> *captures;
   unsigned int nest_depth, bind_indx;
   location_t loc;
   bool saw_capture;
@@ -2869,26 +2887,27 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  local_var_info &local_var
 	    = lvd->local_var_uses->get_or_insert (lvar, &existed);
 	  gcc_checking_assert (!existed);
+	  /* For non-VAR_DECL or var as an alias, just leave it.  */
+	  if (!VAR_P (lvar) || DECL_HAS_VALUE_EXPR_P (lvar))
+	    continue;
 	  tree lvtype = TREE_TYPE (lvar);
 	  tree lvname = DECL_NAME (lvar);
-	  bool captured = is_normal_capture_proxy (lvar);
 	  /* Make names depth+index unique, so that we can support nested
 	     scopes with identically named locals.  */
 	  char *buf;
 	  size_t namsize = sizeof ("__lv...") + 18;
-	  const char *nm = (captured ? "cp" : "lv");
 	  if (lvname != NULL_TREE)
 	    {
 	      namsize += IDENTIFIER_LENGTH (lvname);
 	      buf = (char *) alloca (namsize);
-	      snprintf (buf, namsize, "__%s.%u.%u.%s", nm, lvd->bind_indx,
+	      snprintf (buf, namsize, "__lv.%u.%u.%s", lvd->bind_indx,
 			lvd->nest_depth, IDENTIFIER_POINTER (lvname));
 	    }
 	  else
 	    {
 	      namsize += 10; /* 'D' followed by an unsigned.  */
 	      buf = (char *) alloca (namsize);
-	      snprintf (buf, namsize, "__%s.%u.%u.D%u", nm, lvd->bind_indx,
+	      snprintf (buf, namsize, "__lv.%u.%u.D%u", lvd->bind_indx,
 			lvd->nest_depth, DECL_UID (lvar));
 	    }
 	  /* TODO: Figure out if we should build a local type that has any
@@ -2898,15 +2917,6 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  local_var.def_loc = DECL_SOURCE_LOCATION (lvar);
 	  local_var.frame_type = lvtype;
 	  local_var.field_idx = NULL_TREE;
-	  if (captured)
-	    {
-	      gcc_checking_assert (DECL_INITIAL (lvar) == NULL_TREE);
-	      local_var.captured = lvar;
-	      lvd->captures->safe_push (local_var);
-	      lvd->saw_capture = true;
-	    }
-	  else
-	    local_var.captured = NULL;
 	  lvd->local_var_seen = true;
 	  /* We don't walk any of the local var sub-trees, they won't contain
 	     any bind exprs.  */
@@ -3177,10 +3187,9 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
   /* 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;
-  auto_vec<local_var_info> captures;
 
   local_vars_frame_data local_vars_data
-    = {&field_list, &local_var_uses, &captures, 0, 0, fn_start, false, false};
+    = {&field_list, &local_var_uses, 0, 0, fn_start, false, false};
   cp_walk_tree (&fnbody, register_local_var_uses, &local_vars_data, NULL);
 
   /* Tie off the struct for now, so that we can build offsets to the
@@ -3202,16 +3211,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
   tree coro_fp = build_lang_decl (VAR_DECL, get_identifier ("coro.frameptr"),
 				  coro_frame_ptr);
   tree varlist = coro_fp;
-  local_var_info *cap;
-  if (!captures.is_empty())
-    for (int ix = 0; captures.iterate (ix, &cap); ix++)
-      {
-	if (cap->field_id == NULL_TREE)
-	  continue;
-	tree t = cap->captured;
-	DECL_CHAIN (t) = varlist;
-	varlist = t;
-      }
 
   /* Collected the scope vars we need ... only one for now. */
   BIND_EXPR_VARS (ramp_bind) = nreverse (varlist);
@@ -3477,62 +3476,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	}
     }
 
-  vec<tree, va_gc> *captures_dtor_list = NULL;
-  while (!captures.is_empty())
-    {
-      local_var_info cap = captures.pop();
-      if (cap.field_id == NULL_TREE)
-	continue;
-
-      tree fld_ref = lookup_member (coro_frame_type, cap.field_id,
-				    /*protect=*/1, /*want_type=*/0,
-				    tf_warning_or_error);
-      tree fld_idx
-	= build_class_member_access_expr (deref_fp, fld_ref, NULL_TREE,
-					  false, tf_warning_or_error);
-
-      tree cap_type = cap.frame_type;
-
-      /* When we have a reference, we do not want to change the referenced
-	 item, but actually to set the reference to the proxy var.  */
-      if (REFERENCE_REF_P (fld_idx))
-	fld_idx = TREE_OPERAND (fld_idx, 0);
-
-      if (TYPE_NEEDS_CONSTRUCTING (cap_type))
-	{
-	  vec<tree, va_gc> *p_in;
-	      if (TYPE_REF_P (cap_type)
-		  && (CLASSTYPE_LAZY_MOVE_CTOR (cap_type)
-		      || CLASSTYPE_LAZY_MOVE_ASSIGN (cap_type)
-		      || classtype_has_move_assign_or_move_ctor_p
-			    (cap_type, /*user_declared=*/true)))
-	    p_in = make_tree_vector_single (rvalue (cap.captured));
-	  else
-	    p_in = make_tree_vector_single (cap.captured);
-	  /* Construct in place or move as relevant.  */
-	  r = build_special_member_call (fld_idx, complete_ctor_identifier,
-					 &p_in, cap_type, LOOKUP_NORMAL,
-					 tf_warning_or_error);
-	  release_tree_vector (p_in);
-	  if (captures_dtor_list == NULL)
-	    captures_dtor_list = make_tree_vector ();
-	  vec_safe_push (captures_dtor_list, cap.field_id);
-	}
-      else
-	{
-	  if (!same_type_p (cap_type, TREE_TYPE (cap.captured)))
-	    r = build1_loc (DECL_SOURCE_LOCATION (cap.captured), CONVERT_EXPR,
-			    cap_type, cap.captured);
-	  else
-	    r = cap.captured;
-	  r = build_modify_expr (fn_start, fld_idx, cap_type,
-				 INIT_EXPR, DECL_SOURCE_LOCATION (cap.captured),
-				 r, TREE_TYPE (r));
-	}
-      r = coro_build_cvt_void_expr_stmt (r, fn_start);
-      add_stmt (r);
-    }
-
   /* Set up a new bind context for the GRO.  */
   tree gro_context_bind = build3 (BIND_EXPR, void_type_node, NULL, NULL, NULL);
   /* Make and connect the scope blocks.  */
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C
new file mode 100644
index 00000000000..fb760472368
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C
@@ -0,0 +1,55 @@
+//  { dg-do run }
+
+// lambda with parm and local state
+
+#include "../coro.h"
+
+// boiler-plate for tests of codegen
+#include "../coro1-ret-int-yield-int.h"
+
+int main ()
+{
+  int a_copy = 20;
+
+  auto f = [&a_ref = a_copy, a_copy = a_copy + 10]() -> coro1
+  {
+    a_ref += 20;
+    co_return a_ref + a_copy;
+  };
+
+  {
+    coro1 A = f ();
+    A.handle.resume();
+    if (a_copy != 40)
+      {
+        PRINT ("main: [a_copy = 40]");
+	abort ();
+      }
+  
+    int y = A.handle.promise().get_value();
+    if (y != 70)
+      {
+	PRINTF ("main: A co-ret = %d, should be 30\n", y);
+	abort ();
+      }
+  }
+
+  a_copy = 5;
+
+  coro1 B = f ();
+  B.handle.resume();
+  if (a_copy != 25)
+    {
+      PRINT ("main: [a_copy = 25]");
+      abort ();
+    }
+
+  int y = B.handle.promise().get_value();
+  if (y != 55)
+    {
+      PRINTF ("main: B co-ret = %d, should be 55\n", y);
+      abort ();
+    }
+  
+  return 0;
+}

Hi JunMa,

JunMa <JunMa@linux.alibaba.com> wrote:

> 在 2020/2/4 下午8:17, JunMa 写道:
>> Hi
>> When testing coroutines with lambda function, I find we copy each captured
>> variable to frame. However, according to gimplify pass, for each  
>> declaration
>> that is an alias for another expression(DECL_VALUE_EXPR), we can
>> substitute them directly.
>>
>> Since lambda captured variables is one of this kind. It is better to  
>> replace them
>> rather than copy them, This can reduce frame size (all of the captured  
>> variables
>> are field of closure class) and avoid extra copy behavior as well.
>>
>> This patch remove all of the code related to copy captured variable.
>> Instead, we first rewrite DECL_VALUE_EXPR with frame field, then we check
>> every variable whether it has DECL_VALUE_EXPR, and then substitute it,  
>> this
>> patch does not create frame field for such variables.
>>
>> Bootstrap and test on X86_64, is it OK?

> minor update: only handle var_decl when iterate BIND_EXPR_VARS
> in register_local_var_uses.

Do you have any other local patches applied along with this?

Testing locally (on Darwin), I see regressions with optimisation  O2/O3/Os  
e.g:

class-05-lambda-capture-copy-local.C   -O2  (internal compiler error)
class-06-lambda-capture-ref.C   -O2  (internal compiler error)
lambda-05-capture-copy-local.C   -O2  (internal compiler error)
lambda-06-multi-capture.C   -O2  (internal compiler error)
lambda-07-multi-capture.C   -O2  (internal compiler error)
lambda-08-co-ret-parm-ref.C   -O3 -g  (internal compiler error)

I have applied this to master, and on top of the patches posted by you and
Bin, but the results are the same.

thanks
Iain

>> gcc/cp
>> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>>
>>         * coroutines.cc (morph_fn_to_coro): Remove code related to
>>         copy captured variable.
>>         (register_local_var_uses):  Ditto.
>>         (register_param_uses):  Collect use of parameters inside
>>         DECL_VALUE_EXPR.
>>         (transform_local_var_uses): Substitute the alias variable
>>         with DECL_VALUE_EXPR if it has one.
>>
>>
>> gcc/testsuite
>> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>>
>>         * g++.dg/coroutines/lambda-07-multi-capture.C: New test.
>
>
> <0001-fix-alias-variable.patch>

On Thu, Feb 6, 2020 at 5:12 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi JunMa,
>
> JunMa <JunMa@linux.alibaba.com> wrote:
>
> > 在 2020/2/4 下午8:17, JunMa 写道:
> >> Hi
> >> When testing coroutines with lambda function, I find we copy each captured
> >> variable to frame. However, according to gimplify pass, for each
> >> declaration
> >> that is an alias for another expression(DECL_VALUE_EXPR), we can
> >> substitute them directly.
> >>
> >> Since lambda captured variables is one of this kind. It is better to
> >> replace them
> >> rather than copy them, This can reduce frame size (all of the captured
> >> variables
> >> are field of closure class) and avoid extra copy behavior as well.
> >>
> >> This patch remove all of the code related to copy captured variable.
> >> Instead, we first rewrite DECL_VALUE_EXPR with frame field, then we check
> >> every variable whether it has DECL_VALUE_EXPR, and then substitute it,
> >> this
> >> patch does not create frame field for such variables.
> >>
> >> Bootstrap and test on X86_64, is it OK?
>
> > minor update: only handle var_decl when iterate BIND_EXPR_VARS
> > in register_local_var_uses.
>
> Do you have any other local patches applied along with this?
>
> Testing locally (on Darwin), I see regressions with optimisation  O2/O3/Os
> e.g:
>
> class-05-lambda-capture-copy-local.C   -O2  (internal compiler error)
> class-06-lambda-capture-ref.C   -O2  (internal compiler error)
> lambda-05-capture-copy-local.C   -O2  (internal compiler error)
> lambda-06-multi-capture.C   -O2  (internal compiler error)
> lambda-07-multi-capture.C   -O2  (internal compiler error)
> lambda-08-co-ret-parm-ref.C   -O3 -g  (internal compiler error)
>
> I have applied this to master, and on top of the patches posted by you and
> Bin, but the results are the same.
Hi Iains,

Thanks for helping.
Yes, there will be another patch fixing the O2/O3 issues.  Will send
it out for review soon.

Thanks,
bin
>
> thanks
> Iain
>
> >> gcc/cp
> >> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
> >>
> >>         * coroutines.cc (morph_fn_to_coro): Remove code related to
> >>         copy captured variable.
> >>         (register_local_var_uses):  Ditto.
> >>         (register_param_uses):  Collect use of parameters inside
> >>         DECL_VALUE_EXPR.
> >>         (transform_local_var_uses): Substitute the alias variable
> >>         with DECL_VALUE_EXPR if it has one.
> >>
> >>
> >> gcc/testsuite
> >> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
> >>
> >>         * g++.dg/coroutines/lambda-07-multi-capture.C: New test.
> >
> >
> > <0001-fix-alias-variable.patch>
>
>

在 2020/2/6 下午5:12, Iain Sandoe 写道:
> Hi JunMa,
>
> JunMa <JunMa@linux.alibaba.com> wrote:
>
>> 在 2020/2/4 下午8:17, JunMa 写道:
>>> Hi
>>> When testing coroutines with lambda function, I find we copy each 
>>> captured
>>> variable to frame. However, according to gimplify pass, for each 
>>> declaration
>>> that is an alias for another expression(DECL_VALUE_EXPR), we can
>>> substitute them directly.
>>>
>>> Since lambda captured variables is one of this kind. It is better to 
>>> replace them
>>> rather than copy them, This can reduce frame size (all of the 
>>> captured variables
>>> are field of closure class) and avoid extra copy behavior as well.
>>>
>>> This patch remove all of the code related to copy captured variable.
>>> Instead, we first rewrite DECL_VALUE_EXPR with frame field, then we 
>>> check
>>> every variable whether it has DECL_VALUE_EXPR, and then substitute 
>>> it, this
>>> patch does not create frame field for such variables.
>>>
>>> Bootstrap and test on X86_64, is it OK?
>
>> minor update: only handle var_decl when iterate BIND_EXPR_VARS
>> in register_local_var_uses.
>
> Do you have any other local patches applied along with this?
>
> Testing locally (on Darwin), I see regressions with optimisation 
> O2/O3/Os e.g:
>
> class-05-lambda-capture-copy-local.C   -O2  (internal compiler error)
> class-06-lambda-capture-ref.C   -O2  (internal compiler error)
> lambda-05-capture-copy-local.C   -O2  (internal compiler error)
> lambda-06-multi-capture.C   -O2  (internal compiler error)
> lambda-07-multi-capture.C   -O2  (internal compiler error)
> lambda-08-co-ret-parm-ref.C   -O3 -g  (internal compiler error)
>
> I have applied this to master, and on top of the patches posted by you 
> and
> Bin, but the results are the same.
>
+Bin
This is known issue which has been fixed by Bin, he will send the patch.

Regards
JunMa
> thanks
> Iain
>
>>> gcc/cp
>>> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>>>
>>>         * coroutines.cc (morph_fn_to_coro): Remove code related to
>>>         copy captured variable.
>>>         (register_local_var_uses):  Ditto.
>>>         (register_param_uses):  Collect use of parameters inside
>>>         DECL_VALUE_EXPR.
>>>         (transform_local_var_uses): Substitute the alias variable
>>>         with DECL_VALUE_EXPR if it has one.
>>>
>>>
>>> gcc/testsuite
>>> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>>>
>>>         * g++.dg/coroutines/lambda-07-multi-capture.C: New test.
>>
>>
>> <0001-fix-alias-variable.patch>
>

在 2020/2/6 下午7:09, JunMa 写道:
> 在 2020/2/6 下午5:12, Iain Sandoe 写道:
>> Hi JunMa,
>>
>> JunMa <JunMa@linux.alibaba.com> wrote:
>>
>>> 在 2020/2/4 下午8:17, JunMa 写道:
>>>> Hi
>>>> When testing coroutines with lambda function, I find we copy each 
>>>> captured
>>>> variable to frame. However, according to gimplify pass, for each 
>>>> declaration
>>>> that is an alias for another expression(DECL_VALUE_EXPR), we can
>>>> substitute them directly.
>>>>
>>>> Since lambda captured variables is one of this kind. It is better 
>>>> to replace them
>>>> rather than copy them, This can reduce frame size (all of the 
>>>> captured variables
>>>> are field of closure class) and avoid extra copy behavior as well.
>>>>
>>>> This patch remove all of the code related to copy captured variable.
>>>> Instead, we first rewrite DECL_VALUE_EXPR with frame field, then we 
>>>> check
>>>> every variable whether it has DECL_VALUE_EXPR, and then substitute 
>>>> it, this
>>>> patch does not create frame field for such variables.
>>>>
>>>> Bootstrap and test on X86_64, is it OK?
>>
>>> minor update: only handle var_decl when iterate BIND_EXPR_VARS
>>> in register_local_var_uses.
>>
>> Do you have any other local patches applied along with this?
>>
>> Testing locally (on Darwin), I see regressions with optimisation 
>> O2/O3/Os e.g:
>>
>> class-05-lambda-capture-copy-local.C   -O2  (internal compiler error)
>> class-06-lambda-capture-ref.C   -O2  (internal compiler error)
>> lambda-05-capture-copy-local.C   -O2  (internal compiler error)
>> lambda-06-multi-capture.C   -O2  (internal compiler error)
>> lambda-07-multi-capture.C   -O2  (internal compiler error)
>> lambda-08-co-ret-parm-ref.C   -O3 -g  (internal compiler error)
>>
>> I have applied this to master, and on top of the patches posted by 
>> you and
>> Bin, but the results are the same.
>>
> +Bin
> This is known issue which has been fixed by Bin, he will send the patch.
Hi Iain,

After dig into these regression, I find the patch needs some fix: rather
than replace these alias in front end, we can just leave them to 
gimplify pass.

This change fixes the regressions and simplifies the implementation as well.
Also, no more patches are needed.  Here is the updated patch.


Regards
JunMa
>
> Regards
> JunMa
>> thanks
>> Iain
>>
>>>> gcc/cp
>>>> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>>>>
>>>>         * coroutines.cc (morph_fn_to_coro): Remove code related to
>>>>         copy captured variable.
>>>>         (register_local_var_uses):  Ditto.
>>>>         (register_param_uses):  Collect use of parameters inside
>>>>         DECL_VALUE_EXPR.
>>>>         (transform_local_var_uses): Substitute the alias variable
>>>>         with DECL_VALUE_EXPR if it has one.
>>>>
>>>>
>>>> gcc/testsuite
>>>> 2020-02-04  Jun Ma <JunMa@linux.alibaba.com>
>>>>
>>>>         * g++.dg/coroutines/lambda-07-multi-capture.C: New test.
>>>
>>>
>>> <0001-fix-alias-variable.patch>
>>
---
 gcc/cp/coroutines.cc                          | 112 ++++--------------
 .../torture/lambda-07-multi-capture.C         |  55 +++++++++
 2 files changed, 79 insertions(+), 88 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 0879d57b060..decec4550af 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -1793,6 +1793,11 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  cp_walk_tree (&DECL_SIZE (lvar), transform_local_var_uses, d, NULL);
 	  cp_walk_tree (&DECL_SIZE_UNIT (lvar), transform_local_var_uses, d,
 			NULL);
+	  if (VAR_P (lvar) && DECL_HAS_VALUE_EXPR_P (lvar))
+	    {
+	      tree t  = DECL_VALUE_EXPR (lvar);
+	      cp_walk_tree (&t, transform_local_var_uses, d, NULL);
+	    }
 
 	  /* TODO: implement selective generation of fields when vars are
 	     known not-used.  */
@@ -1818,9 +1823,9 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  gcc_checking_assert (existed);
 
 	  if (local_var.field_id == NULL_TREE)
-	    pvar = &DECL_CHAIN (*pvar); /* Wasn't used.  */
-
-	  *pvar = DECL_CHAIN (*pvar); /* discard this one, we replaced it.  */
+	    pvar = &DECL_CHAIN (*pvar); /* Wasn't used or was an alias.  */
+	  else
+	    *pvar = DECL_CHAIN (*pvar); /* discard this one, we replaced it.  */
 	}
 
       *do_subtree = 0; /* We've done the body already.  */
@@ -1844,10 +1849,11 @@ transform_local_var_uses (tree *stmt, int *do_subtree, void *d)
     return NULL_TREE;
 
   /* VAR_DECLs that are not recorded can belong to the proxies we've placed
-     for the promise and coroutine handle(s), to global vars or to compiler
-     temporaries.  Skip past these, we will handle them later.  */
+     for the promise and coroutine handle(s), to global vars, to compiler
+     temporaries or to vars as alias.  Skip past these, we will handle
+     them later.  */
   local_var_info *local_var_i = lvd->local_var_uses->get (var_decl);
-  if (local_var_i == NULL)
+  if (local_var_i == NULL || local_var_i->field_id == NULL_TREE)
     return NULL_TREE;
 
   /* This is our revised 'local' i.e. a frame slot.  */
@@ -2797,6 +2803,12 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d)
 {
   param_frame_data *data = (param_frame_data *) d;
 
+  if (VAR_P (*stmt) && DECL_HAS_VALUE_EXPR_P (*stmt))
+    {
+      tree x = DECL_VALUE_EXPR (*stmt);
+      cp_walk_tree (&x, register_param_uses, d, NULL);
+    }
+
   if (TREE_CODE (*stmt) != PARM_DECL)
     return NULL_TREE;
 
@@ -2841,7 +2853,6 @@ struct local_vars_frame_data
 {
   tree *field_list;
   hash_map<tree, local_var_info> *local_var_uses;
-  vec<local_var_info> *captures;
   unsigned int nest_depth, bind_indx;
   location_t loc;
   bool saw_capture;
@@ -2870,26 +2881,27 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  local_var_info &local_var
 	    = lvd->local_var_uses->get_or_insert (lvar, &existed);
 	  gcc_checking_assert (!existed);
+	  /* For non-VAR_DECL or var as an alias, just leave it.  */
+	  if (!VAR_P (lvar) || DECL_HAS_VALUE_EXPR_P (lvar))
+	    continue;
 	  tree lvtype = TREE_TYPE (lvar);
 	  tree lvname = DECL_NAME (lvar);
-	  bool captured = is_normal_capture_proxy (lvar);
 	  /* Make names depth+index unique, so that we can support nested
 	     scopes with identically named locals.  */
 	  char *buf;
 	  size_t namsize = sizeof ("__lv...") + 18;
-	  const char *nm = (captured ? "cp" : "lv");
 	  if (lvname != NULL_TREE)
 	    {
 	      namsize += IDENTIFIER_LENGTH (lvname);
 	      buf = (char *) alloca (namsize);
-	      snprintf (buf, namsize, "__%s.%u.%u.%s", nm, lvd->bind_indx,
+	      snprintf (buf, namsize, "__lv.%u.%u.%s", lvd->bind_indx,
 			lvd->nest_depth, IDENTIFIER_POINTER (lvname));
 	    }
 	  else
 	    {
 	      namsize += 10; /* 'D' followed by an unsigned.  */
 	      buf = (char *) alloca (namsize);
-	      snprintf (buf, namsize, "__%s.%u.%u.D%u", nm, lvd->bind_indx,
+	      snprintf (buf, namsize, "__lv.%u.%u.D%u", lvd->bind_indx,
 			lvd->nest_depth, DECL_UID (lvar));
 	    }
 	  /* TODO: Figure out if we should build a local type that has any
@@ -2899,15 +2911,6 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
 	  local_var.def_loc = DECL_SOURCE_LOCATION (lvar);
 	  local_var.frame_type = lvtype;
 	  local_var.field_idx = NULL_TREE;
-	  if (captured)
-	    {
-	      gcc_checking_assert (DECL_INITIAL (lvar) == NULL_TREE);
-	      local_var.captured = lvar;
-	      lvd->captures->safe_push (local_var);
-	      lvd->saw_capture = true;
-	    }
-	  else
-	    local_var.captured = NULL;
 	  lvd->local_var_seen = true;
 	  /* We don't walk any of the local var sub-trees, they won't contain
 	     any bind exprs.  */
@@ -3178,10 +3181,9 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
   /* 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;
-  auto_vec<local_var_info> captures;
 
   local_vars_frame_data local_vars_data
-    = {&field_list, &local_var_uses, &captures, 0, 0, fn_start, false, false};
+    = {&field_list, &local_var_uses, 0, 0, fn_start, false, false};
   cp_walk_tree (&fnbody, register_local_var_uses, &local_vars_data, NULL);
 
   /* Tie off the struct for now, so that we can build offsets to the
@@ -3203,16 +3205,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
   tree coro_fp = build_lang_decl (VAR_DECL, get_identifier ("coro.frameptr"),
 				  coro_frame_ptr);
   tree varlist = coro_fp;
-  local_var_info *cap;
-  if (!captures.is_empty())
-    for (int ix = 0; captures.iterate (ix, &cap); ix++)
-      {
-	if (cap->field_id == NULL_TREE)
-	  continue;
-	tree t = cap->captured;
-	DECL_CHAIN (t) = varlist;
-	varlist = t;
-      }
 
   /* Collected the scope vars we need ... only one for now. */
   BIND_EXPR_VARS (ramp_bind) = nreverse (varlist);
@@ -3478,62 +3470,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	}
     }
 
-  vec<tree, va_gc> *captures_dtor_list = NULL;
-  while (!captures.is_empty())
-    {
-      local_var_info cap = captures.pop();
-      if (cap.field_id == NULL_TREE)
-	continue;
-
-      tree fld_ref = lookup_member (coro_frame_type, cap.field_id,
-				    /*protect=*/1, /*want_type=*/0,
-				    tf_warning_or_error);
-      tree fld_idx
-	= build_class_member_access_expr (deref_fp, fld_ref, NULL_TREE,
-					  false, tf_warning_or_error);
-
-      tree cap_type = cap.frame_type;
-
-      /* When we have a reference, we do not want to change the referenced
-	 item, but actually to set the reference to the proxy var.  */
-      if (REFERENCE_REF_P (fld_idx))
-	fld_idx = TREE_OPERAND (fld_idx, 0);
-
-      if (TYPE_NEEDS_CONSTRUCTING (cap_type))
-	{
-	  vec<tree, va_gc> *p_in;
-	      if (TYPE_REF_P (cap_type)
-		  && (CLASSTYPE_LAZY_MOVE_CTOR (cap_type)
-		      || CLASSTYPE_LAZY_MOVE_ASSIGN (cap_type)
-		      || classtype_has_move_assign_or_move_ctor_p
-			    (cap_type, /*user_declared=*/true)))
-	    p_in = make_tree_vector_single (rvalue (cap.captured));
-	  else
-	    p_in = make_tree_vector_single (cap.captured);
-	  /* Construct in place or move as relevant.  */
-	  r = build_special_member_call (fld_idx, complete_ctor_identifier,
-					 &p_in, cap_type, LOOKUP_NORMAL,
-					 tf_warning_or_error);
-	  release_tree_vector (p_in);
-	  if (captures_dtor_list == NULL)
-	    captures_dtor_list = make_tree_vector ();
-	  vec_safe_push (captures_dtor_list, cap.field_id);
-	}
-      else
-	{
-	  if (!same_type_p (cap_type, TREE_TYPE (cap.captured)))
-	    r = build1_loc (DECL_SOURCE_LOCATION (cap.captured), CONVERT_EXPR,
-			    cap_type, cap.captured);
-	  else
-	    r = cap.captured;
-	  r = build_modify_expr (fn_start, fld_idx, cap_type,
-				 INIT_EXPR, DECL_SOURCE_LOCATION (cap.captured),
-				 r, TREE_TYPE (r));
-	}
-      r = coro_build_cvt_void_expr_stmt (r, fn_start);
-      add_stmt (r);
-    }
-
   /* Set up a new bind context for the GRO.  */
   tree gro_context_bind = build3 (BIND_EXPR, void_type_node, NULL, NULL, NULL);
   /* Make and connect the scope blocks.  */
diff --git a/gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C b/gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C
new file mode 100644
index 00000000000..fb760472368
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/torture/lambda-07-multi-capture.C
@@ -0,0 +1,55 @@
+//  { dg-do run }
+
+// lambda with parm and local state
+
+#include "../coro.h"
+
+// boiler-plate for tests of codegen
+#include "../coro1-ret-int-yield-int.h"
+
+int main ()
+{
+  int a_copy = 20;
+
+  auto f = [&a_ref = a_copy, a_copy = a_copy + 10]() -> coro1
+  {
+    a_ref += 20;
+    co_return a_ref + a_copy;
+  };
+
+  {
+    coro1 A = f ();
+    A.handle.resume();
+    if (a_copy != 40)
+      {
+        PRINT ("main: [a_copy = 40]");
+	abort ();
+      }
+  
+    int y = A.handle.promise().get_value();
+    if (y != 70)
+      {
+	PRINTF ("main: A co-ret = %d, should be 30\n", y);
+	abort ();
+      }
+  }
+
+  a_copy = 5;
+
+  coro1 B = f ();
+  B.handle.resume();
+  if (a_copy != 25)
+    {
+      PRINT ("main: [a_copy = 25]");
+      abort ();
+    }
+
+  int y = B.handle.promise().get_value();
+  if (y != 55)
+    {
+      PRINTF ("main: B co-ret = %d, should be 55\n", y);
+      abort ();
+    }
+  
+  return 0;
+}