From: Richard Biener <rguenther@suse.de>
To: Qing Zhao <qing.zhao@oracle.com>
Cc: "jason@redhat.com" <jason@redhat.com>,
gcc-patches Nick Alcock via <gcc-patches@gcc.gnu.org>
Subject: Re: [patch][middle-end/PR102359]Not add initialization for DECL_VALUE_EXPR variables with -ftrivial-auto-var-init
Date: Tue, 5 Oct 2021 10:19:12 +0200 (CEST) [thread overview]
Message-ID: <2r53651-n1q1-2rp7-p316-o330p2sp161o@fhfr.qr> (raw)
In-Reply-To: <0FFDE9ED-74C7-4A66-8DD1-2AD8B55AB303@oracle.com>
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 ();
> +}
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
next prev parent reply other threads:[~2021-10-05 8:19 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-05 4:52 Qing Zhao
2021-10-05 8:19 ` Richard Biener [this message]
2021-10-05 14:32 ` Qing Zhao
2021-10-05 15:36 ` Qing Zhao
2021-10-05 18:30 ` Jason Merrill
2021-10-05 22:39 ` Qing Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2r53651-n1q1-2rp7-p316-o330p2sp161o@fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=qing.zhao@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).