| Message ID | 0FFDE9ED-74C7-4A66-8DD1-2AD8B55AB303@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [middle-end/PR102359] Not add initialization for DECL_VALUE_EXPR variables with -ftrivial-auto-var-init | expand |
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 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; + 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) + && !decl_had_value_expr_p) { gimple_add_init_for_auto_var (decl, flag_auto_var_init, OK with that change. 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 (); > +} >
> 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)
This is the updated patch, I will push it after done with testing.
Let me know if you see any issue.
Thanks.
Qing.
============================
From 1f07c20ecdc9a015369c8bb472ebbd3bcaabdf17 Mon Sep 17 00:00:00 2001
From: qing zhao <qing.zhao@oracle.com>
Date: Tue, 5 Oct 2021 08:10:02 -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-05 qing zhao <qing.zhao@oracle.com>
* gimplify.c (gimplify_decl_expr): Not add initialization for an
auto variable when it has been initialized by frontend.
gcc/testsuite/ChangeLog:
2021-10-05 qing zhao <qing.zhao@oracle.com>
* g++.dg/pr102359_1.C: New test.
* g++.dg/pr102359_2.C: New test.
---
gcc/gimplify.c | 9 ++++++++-
gcc/testsuite/g++.dg/pr102359_1.C | 13 +++++++++++++
gcc/testsuite/g++.dg/pr102359_2.C | 13 +++++++++++++
3 files changed, 34 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..d96fca4 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1872,6 +1872,9 @@ 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 a 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 +1937,11 @@ 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 decl has VALUE_EXPR that was created by FE (usually
+ C++FE), it's a proxy varaible, and FE already initialized
+ the VALUE_EXPR of it, we should not initialize it here. */
+ && !decl_had_value_expr_p)
{
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 ();
+}
On 10/5/21 10:32, Qing Zhao wrote: > > >> 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. */ I'd suggest merging this comment into the comment on the variable declaration. >> + && !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) >
> On Oct 5, 2021, at 1:30 PM, Jason Merrill <jason@redhat.com> wrote: > >>> '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. */ > > I'd suggest merging this comment into the comment on the variable declaration. Okay, will merge these two comments into one and put it on the variable declaration. thanks. Qing
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; 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); + 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)) { 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 (); +}
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