c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426].

  The changes to fix PR 105287 included a tightening of the constraints on which
variables are promoted to frame copies.  This has exposed that we are failing
to name some variables that should be promoted.

The long-term fix is to address the cases where the naming has been missed,
but for the short-term (and for the GCC-12 branch) backing out the additional
constraint is proposed.

This makes the fix for PR 105287 more conservative.

tested on x86_64-darwin, with the reproducer from the PR which now produces
the correct output for both optimised and unoptimised code.

OK for master?
OK for GCC-12?
thanks,
Iain

Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

	PR c++/105426

gcc/cp/ChangeLog:

	* coroutines.cc (register_local_var_uses): Allow promotion of unnamed
	temporaries to coroutine frame copies.
---
 gcc/cp/coroutines.cc | 3 +++
 1 file changed, 3 insertions(+)
  

On Thu, Apr 28, 2022 at 08:17:04PM +0100, Iain Sandoe wrote:
> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
> 
> 	PR c++/105426
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (register_local_var_uses): Allow promotion of unnamed
> 	temporaries to coroutine frame copies.
> ---
>  gcc/cp/coroutines.cc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 551ddc9cc41..2e393b2cddc 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>  	  else if (lvname != NULL_TREE)
>  	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>  			     lvd->nest_depth, lvd->bind_indx);
> +	  else
> +	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
> +			     lvd->bind_indx);
>  	  /* TODO: Figure out if we should build a local type that has any
>  	     excess alignment or size from the original decl.  */
>  	  if (buf)

This isn't going to play well with -fcompare-debug.
We don't guarantee same DECL_UID values between -g and -g0, just that
when two decls are created with both -g and -g0 they compare the same,
but with -g the gap in between them could be larger.
So names that include DECL_UID will be often different between -g and -g0.

Can't the FIELD_DECL be instead nameless?  Say change coro_make_frame_entry
to do
  tree id = name ? get_identifier (name) : NULL_TREE;
instead of
  tree id = get_identifier (name);
?

	Jakub
  

> On 28 Apr 2022, at 20:26, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Thu, Apr 28, 2022 at 08:17:04PM +0100, Iain Sandoe wrote:
>> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
>> 
>> 	PR c++/105426
>> 
>> gcc/cp/ChangeLog:
>> 
>> 	* coroutines.cc (register_local_var_uses): Allow promotion of unnamed
>> 	temporaries to coroutine frame copies.
>> ---
>> gcc/cp/coroutines.cc | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>> index 551ddc9cc41..2e393b2cddc 100644
>> --- a/gcc/cp/coroutines.cc
>> +++ b/gcc/cp/coroutines.cc
>> @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>> 	  else if (lvname != NULL_TREE)
>> 	    buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>> 			     lvd->nest_depth, lvd->bind_indx);
>> +	  else
>> +	    buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
>> +			     lvd->bind_indx);
>> 	  /* TODO: Figure out if we should build a local type that has any
>> 	     excess alignment or size from the original decl.  */
>> 	  if (buf)
> 
> This isn't going to play well with -fcompare-debug.
> We don't guarantee same DECL_UID values between -g and -g0, just that
> when two decls are created with both -g and -g0 they compare the same,
> but with -g the gap in between them could be larger.
> So names that include DECL_UID will be often different between -g and -g0.

that’s somewhat unfortunate (having the UIDs in the frame and the gimple is quite
helpful to debugging).  I guess I was not expecting a difference this early in the FE
(we are before initial folding / constexpr stuff).

FWIW, I ran the coroutines and coroutines torture testsuites with -fcompare-debug.
There are 5 fails, and those are unaffected by whether the field is made nameless as
suggested below (so something else to investigate).

> Can't the FIELD_DECL be instead nameless?  Say change coro_make_frame_entry
> to do
>  tree id = name ? get_identifier (name) : NULL_TREE;
> instead of
>  tree id = get_identifier (name);

Unfortunately, that does not work - although I cannot see anything in the coroutines code
that would cause it to fail - perhaps something is not playing nicely with
DECL_VALUE_EXPRs on unnamed temps?

how about the following, which uniques the names by bind scope, scope nest and then
sequence within that?

Iain

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 2e393b2cddc..1d886b31c77 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -3913,6 +3913,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
   if (TREE_CODE (*stmt) == BIND_EXPR)
     {
       tree lvar;
+      unsigned serial = 0;
       for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL;
           lvar = DECL_CHAIN (lvar))
        {
@@ -3974,16 +3975,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
            buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
                             lvd->nest_depth, lvd->bind_indx);
          else
-           buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
-                            lvd->bind_indx);
+           buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx,
+                            serial++);
+
          /* TODO: Figure out if we should build a local type that has any
             excess alignment or size from the original decl.  */
-         if (buf)
-           {
-             local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
-                                                         lvtype, lvd->loc);
-             free (buf);
-           }
+         local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
+                                                     lvtype, lvd->loc);
+         free (buf);
          /* We don't walk any of the local var sub-trees, they won't contain
             any bind exprs.  */
        }
  

On Thu, Apr 28, 2022 at 11:22:45PM +0100, Iain Sandoe wrote:
> how about the following, which uniques the names by bind scope, scope nest and then
> sequence within that?

That LGTM.

> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -3913,6 +3913,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>    if (TREE_CODE (*stmt) == BIND_EXPR)
>      {
>        tree lvar;
> +      unsigned serial = 0;
>        for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL;
>            lvar = DECL_CHAIN (lvar))
>         {
> @@ -3974,16 +3975,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d)
>             buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname),
>                              lvd->nest_depth, lvd->bind_indx);
>           else
> -           buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth,
> -                            lvd->bind_indx);
> +           buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx,
> +                            serial++);
> +
>           /* TODO: Figure out if we should build a local type that has any
>              excess alignment or size from the original decl.  */
> -         if (buf)
> -           {
> -             local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
> -                                                         lvtype, lvd->loc);
> -             free (buf);
> -           }
> +         local_var.field_id = coro_make_frame_entry (lvd->field_list, buf,
> +                                                     lvtype, lvd->loc);
> +         free (buf);
>           /* We don't walk any of the local var sub-trees, they won't contain
>              any bind exprs.  */
>         }
> 
> 
> 

	Jakub