From: Qing Zhao <qing.zhao@oracle.com>
To: Richard Biener <rguenther@suse.de>,
"Jason@redhat.com" <Jason@redhat.com>
Cc: gcc-patches Nick Alcock via <gcc-patches@gcc.gnu.org>
Subject: Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init
Date: Thu, 30 Sep 2021 15:42:06 +0000 [thread overview]
Message-ID: <710E24B6-B845-49F0-B426-741905C48EA2@oracle.com> (raw)
In-Reply-To: <5q583245-3qq5-76p7-o1p4-312496os4140@fhfr.qr>
> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
>
> On Thu, 30 Sep 2021, Jason Merrill wrote:
>
>> On 9/29/21 17:30, Qing Zhao wrote:
>>> Hi,
>>>
>>> PR102359 (ICE gimplification failed since r12-3433-ga25e0b5e6ac8a77a)
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>>
>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>> variable “this” in the following routine: (t.cpp.005t.original)
>>>
>>> =======
>>>
>>> ;; Function A::foo()::<lambda()> (null)
>>> ;; enabled by -tree-original
>>>
>>> {
>>> const struct A * const this [value-expr: &__closure->__this];
>>> const struct A * const this [value-expr: &__closure->__this];
>>> return <retval> = (double) ((const struct A *) this)->a;
>>> }
>>> =======
>>>
>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>> value-expr "&__closure->__this” is marked as READONLY.
>>>
>>> There are two major issues:
>>>
>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>> marked as READONLY;
>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>>
>>> The idea solution will be:
>>>
>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>>
>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>>
>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>> middle end:
>>>
>>> Let me know your comments or suggestions on this.
>>>
>>> Thanks a lot for the help.
>>
>> I'd think is_var_need_auto_init should be false for any variable with
>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>> objects that are initialized elsewhere.
>
> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
> auto-init VLAs, otherwise I tend to agree - would we handle those
> when we see a DECL_EXPR then?
The current implementation is:
gimplify_decl_expr:
For each DECL_EXPR “decl”
If (VAR_P (decl) && !DECL_EXTERNAL (decl))
{
if (is_vla (decl))
gimplify_vla_decl (decl, …); /* existing handling: create a VALUE_EXPR for this vla decl*/
…
if (has_explicit_init (decl))
{
…; /* existing handling. */
}
else if (is_var_need_auto_init (decl)) /*. New code. */
{
gimple_add_init_for_auto_var (….); /* new code. */
...
}
}
Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either. We will miss adding initializations for these decls.
So, I think that the current implementation is correct.
And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
Let me know your opinion on this.
Thanks.
Qing
>
>>
>>> Qing
>>>
>>> ==============================
>>> From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
>>> From: Qing Zhao <qing.zhao@oracle.com>
>>> Date: Wed, 29 Sep 2021 20:49:59 +0000
>>> Subject: [PATCH] Fix PR102359
>>>
>>> ---
>>> gcc/gimplify.c | 15 +++++++++++++++
>>> gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
>>> 2 files changed, 28 insertions(+)
>>> create mode 100644 gcc/testsuite/g++.dg/pr102359.C
>>>
>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>> index 1067113b1639..a2587869b35d 100644
>>> --- a/gcc/gimplify.c
>>> +++ b/gcc/gimplify.c
>>> @@ -1819,12 +1819,27 @@ 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 READONLY.
>>> + This is to workaround a C++ FE bug that only mark the value_expr of
>>> "this"
>>> + as readonly but does not mark "this" as readonly.
>>> + C++ FE should fix this issue before replacing this routine with
>>> + TREE_READONLY (decl). */
>>> +
>>> +static bool
>>> +is_decl_readonly (tree decl)
>>> +{
>>> + return (TREE_READONLY (decl)
>>> + || (DECL_HAS_VALUE_EXPR_P (decl)
>>> + && TREE_READONLY (DECL_VALUE_EXPR (decl))));
>>> +}
>>> +
>>> /* Return true if the DECL need to be automaticly initialized by the
>>> compiler. */
>>> static bool
>>> is_var_need_auto_init (tree decl)
>>> {
>>> if (auto_var_p (decl)
>>> + && !is_decl_readonly (decl)
>>> && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
>>> && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
>>> && !is_empty_type (TREE_TYPE (decl)))
>>> diff --git a/gcc/testsuite/g++.dg/pr102359.C
>>> b/gcc/testsuite/g++.dg/pr102359.C
>>> new file mode 100644
>>> index 000000000000..da643cde7bed
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr102359.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()();
>>>
>>
>>
>
> --
> 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-09-30 15:43 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-29 21:30 Qing Zhao
2021-09-30 4:27 ` Jason Merrill
2021-09-30 6:54 ` Richard Biener
2021-09-30 14:15 ` Qing Zhao
2021-09-30 15:42 ` Qing Zhao [this message]
2021-09-30 19:31 ` Jason Merrill
2021-10-01 14:54 ` Qing Zhao
2021-10-01 15:33 ` Jason Merrill
2021-10-01 15:48 ` Qing Zhao
2021-10-04 6:44 ` Richard Biener
2021-10-04 14:07 ` Qing Zhao
2021-09-30 16:46 ` 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=710E24B6-B845-49F0-B426-741905C48EA2@oracle.com \
--to=qing.zhao@oracle.com \
--cc=Jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=rguenther@suse.de \
/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).