Re: [patch][middle-end/PR102359]Not add initialization for DECL_VALUE_EXPR variables with -ftrivial-auto-var-init

[Qing Zhao <qing.zhao@oracle.com>]

> On Oct 5, 2021, at 3:19 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Tue, 5 Oct 2021, Qing Zhao wrote:
> 
>> Hi, 
>> 
>> This is the patch to fix this issue based on our discussion.
>> 
>> I have tested it on aarch64 with bootstrap and regtests. X86 bootstrap was done, regtests is ongoing.
>> 
>> Okay for trunk?
>> 
>> Thanks.
>> 
>> Qing
>> 
>> ======================
>> From d349ef0145512efe7f9af2c6bbd01f636475bce3 Mon Sep 17 00:00:00 2001
>> From: qing zhao <qing.zhao@oracle.com>
>> Date: Mon, 4 Oct 2021 15:26:03 -0700
>> Subject: [PATCH] middle-end/102359 Not add initialization for variables that
>> have been  initialized by FEs.
>> 
>> C++ FE creates proxy variables, which have associated DECL_VALUE_EXPR
>> and have been initialized by FE. For such auto variable, we should not
>> add initialization when -ftrivial-auto-var-init presents.
>> 
>> gcc/ChangeLog:
>> 
>> 2021-10-04  qing zhao  <qing.zhao@oracle.com>
>> 
>> 	* gimplify.c (is_decl_init_by_fe): New function.
>> 	(gimplify_decl_expr): Not add initialization for an auto variable
>> 	when it has been initialized by frontend.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 2021-10-04  qing zhao  <qing.zhao@oracle.com>
>> 
>> 	* g++.dg/pr102359_1.C: New test.
>> 	* g++.dg/pr102359_2.C: New test.
>> ---
>> gcc/gimplify.c                    | 21 ++++++++++++++++++++-
>> gcc/testsuite/g++.dg/pr102359_1.C | 13 +++++++++++++
>> gcc/testsuite/g++.dg/pr102359_2.C | 13 +++++++++++++
>> 3 files changed, 46 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/g++.dg/pr102359_1.C
>> create mode 100644 gcc/testsuite/g++.dg/pr102359_2.C
>> 
>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>> index b27776a..d6865ad 100644
>> --- a/gcc/gimplify.c
>> +++ b/gcc/gimplify.c
>> @@ -1819,6 +1819,19 @@ gimple_add_padding_init_for_auto_var (tree decl, bool is_vla,
>>   gimplify_seq_add_stmt (seq_p, call);
>> }
>> 
>> +/* Return true if the DECL is initialized by FE.
>> +   If the VAR_DECL has DECL_VALUE_EXPR that was created by FE (usually C++FE),
>> +   it's a proxy varaible, and FE already initializd the DECL_VALUE_EXPR of it.
>> +*/
>> +static bool
>> +is_decl_init_by_fe (tree decl, bool is_created_by_fe)
>> +{
>> +  if (DECL_HAS_VALUE_EXPR_P (decl)
>> +      && is_created_by_fe)
>> +    return true;
>> +  return false;
>> +}
>> +
>> /* Return true if the DECL need to be automaticly initialized by the
>>    compiler.  */
>> static bool
>> @@ -1871,8 +1884,13 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>>   if (VAR_P (decl) && !DECL_EXTERNAL (decl))
>>     {
>>       tree init = DECL_INITIAL (decl);
>> +      bool is_value_expr_created_by_fe = false;
> 
> no need for the = false, it's always initialized below.
> 
>>       bool is_vla = false;
>> 
>> +      /* Check whether a decl has FE created VALUE_EXPR here BEFORE 
>> +	 gimplify_vla_decl creates VALUE_EXPR for vla decl.  */
>> +      is_value_expr_created_by_fe = DECL_HAS_VALUE_EXPR_P (decl);
> 
> That looks a bit weird when looking at ...
> 
>> +
>>       poly_uint64 size;
>>       if (!poly_int_tree_p (DECL_SIZE_UNIT (decl), &size)
>> 	  || (!TREE_STATIC (decl)
>> @@ -1934,7 +1952,8 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>>       /* When there is no explicit initializer, if the user requested,
>> 	 We should insert an artifical initializer for this automatic
>> 	 variable.  */
>> -      else if (is_var_need_auto_init (decl))
>> +      else if (is_var_need_auto_init (decl)
>> +	       && !is_decl_init_by_fe (decl, is_value_expr_created_by_fe))
> 
> ... which just expands to
> 
> if (DECL_HAS_VALUE_EXPR_P (decl) && DECL_HAS_VALUE_EXPR_P (decl))
> 
> can you please name 'is_value_expr_created_by_fe' as
> 'decl_had_value_expr_p' and check && !decl_had_value_expr_p here?
> So sth like

I can do this -:) I agree that the change will make the code simpler.

However, my major concern with this change is: later when people look at this change, they might ask:
Why we should not initialize a variable with VALUE_EXPR? And whether the variable whose VALUE_EXPR 
was created by “gimplify_vla_decl” should be excluded? 

My new function and comments were all for this purpose.

If I go with this change, at least we should add some comments to explain this as following, what do you think?
> 
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index b27776af7c8..9013f385f13 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1872,6 +1872,7 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>     {
>       tree init = DECL_INITIAL (decl);
>       bool is_vla = false;

   + /* Check whether a decl has FE created VALUE_EXPR here BEFORE 
   +	 gimplify_vla_decl creates VALUE_EXPR for vla decl.  */

> +      bool decl_had_value_expr_p = DECL_HAS_VALUE_EXPR_P (decl);
> 
>       poly_uint64 size;
>       if (!poly_int_tree_p (DECL_SIZE_UNIT (decl), &size)
> @@ -1934,7 +1935,8 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>       /* When there is no explicit initializer, if the user requested,
>         We should insert an artifical initializer for this automatic
>         variable.  */
> -      else if (is_var_need_auto_init (decl))
> +      else if (is_var_need_auto_init (decl)

+   /* If the VAR_DECL has DECL_VALUE_EXPR that was created by FE (usually C++FE),
+   it's a proxy varaible, and FE already initializd the DECL_VALUE_EXPR of it. */

> +              && !decl_had_value_expr_p)
>        {
>          gimple_add_init_for_auto_var (decl,
>                                        flag_auto_var_init,
> 
> OK with that change.

thanks.

Qing
> 
> Thanks,
> Richard.
> 
> 
>> 	{
>> 	  gimple_add_init_for_auto_var (decl,
>> 					flag_auto_var_init,
>> diff --git a/gcc/testsuite/g++.dg/pr102359_1.C b/gcc/testsuite/g++.dg/pr102359_1.C
>> new file mode 100644
>> index 0000000..da643cd
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr102359_1.C
>> @@ -0,0 +1,13 @@
>> +/* PR middle-end/102359 ICE gimplification failed since
>> +   r12-3433-ga25e0b5e6ac8a77a.  */
>> +/* { dg-do compile } */
>> +/* { dg-options "-ftrivial-auto-var-init=zero" } */
>> +/* { dg-require-effective-target c++17 } */
>> +
>> +struct A {
>> +  double a = 111;
>> +  auto foo() {
>> +    return [*this] { return a; };
>> +  }
>> +};
>> +int X = A{}.foo()();
>> diff --git a/gcc/testsuite/g++.dg/pr102359_2.C b/gcc/testsuite/g++.dg/pr102359_2.C
>> new file mode 100644
>> index 0000000..d026d72
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/pr102359_2.C
>> @@ -0,0 +1,13 @@
>> +/* PR middle-end/102359 ICE gimplification failed since
>> +   r12-3433-ga25e0b5e6ac8a77a.  */
>> +/* { dg-do run} */
>> +/* { dg-options "-ftrivial-auto-var-init=zero" } */
>> +/* { dg-require-effective-target c++17 } */
>> +
>> +int main()
>> +{
>> + int i = 42;
>> + auto l = [=]() mutable { return i; };
>> + if (l() != i)
>> +   __builtin_abort ();
>> +}
>> 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)