From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id BF54E3858C39 for ; Fri, 29 Oct 2021 07:21:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BF54E3858C39 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 837321FD60; Fri, 29 Oct 2021 07:21:41 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 7C1C3A3B81; Fri, 29 Oct 2021 07:21:41 +0000 (UTC) Date: Fri, 29 Oct 2021 09:21:41 +0200 (CEST) From: Richard Biener To: Qing Zhao cc: jakub Jelinek , gcc-patches Nick Alcock via Subject: Re: [Version 2][Patch][PR102281]do not add BUILTIN_CLEAR_PADDING for variables that are gimple registers In-Reply-To: Message-ID: <4rpp251s-613n-n335-n75q-ps6q563p5133@fhfr.qr> References: <4B97F196-DE1A-4E67-96A6-5A0E4AC3859A@oracle.com> <11A177F3-D339-4D2B-8D8E-96539AFCD938@oracle.com> MIME-Version: 1.0 X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Oct 2021 07:21:45 -0000 On Thu, 28 Oct 2021, Qing Zhao wrote: > Ping…. > > Hi, > > Based on the previous discussion, I thought that we have agreed that the proposed patch for this current bug is the correct fix. > And This bug is an important bug that need to be fixed. > > I have created another new PR for the other potential issue with padding initialization for long double/_Complex long double variables with explicit initializer https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102781, and will be fixed separately later if needed. > > Please take a look of the new patch and let me know whether there is any > more issue with this version? Or it’s okay for commit now? I think it's reasonable. Thus OK unless Jakub has comments. Thanks, Richard. > Thanks. > > Qing > > > > > On Oct 25, 2021, at 9:16 AM, Qing Zhao via Gcc-patches wrote: > > > > Ping…. > > > > Is this Okay for trunk? > > > >> On Oct 18, 2021, at 2:26 PM, Qing Zhao via Gcc-patches wrote: > >> > >> Hi, Jakub, > >> > >> This is the 2nd version of the patch based on your comment. > >> > >> Bootstrapped on both x86 and aarch64. Regression testings are ongoing. > > > > The regression testing looks good. > > > > Thanks. > > > > Qing > >> > >> Please let me know if this is ready for committing? > >> > >> Thanks a lot. > >> > >> Qing. > >> > >> ====================== > >> > >> From d6f60370dee69b5deb3d7ef51873a5e986490782 Mon Sep 17 00:00:00 2001 > >> From: Qing Zhao > >> Date: Mon, 18 Oct 2021 19:04:39 +0000 > >> Subject: [PATCH] PR 102281 (-ftrivial-auto-var-init=zero causes ice) > >> > >> Do not add call to __builtin_clear_padding when a variable is a gimple > >> register or it might not have padding. > >> > >> gcc/ChangeLog: > >> > >> 2021-10-18 qing zhao > >> > >> * gimplify.c (gimplify_decl_expr): Do not add call to > >> __builtin_clear_padding when a variable is a gimple register > >> or it might not have padding. > >> (gimplify_init_constructor): Likewise. > >> > >> gcc/testsuite/ChangeLog: > >> > >> 2021-10-18 qing zhao > >> > >> * c-c++-common/pr102281.c: New test. > >> * gcc.target/i386/auto-init-2.c: Adjust testing case. > >> * gcc.target/i386/auto-init-4.c: Likewise. > >> * gcc.target/i386/auto-init-6.c: Likewise. > >> * gcc.target/aarch64/auto-init-6.c: Likewise. > >> --- > >> gcc/gimplify.c | 25 ++++++++++++++----- > >> gcc/testsuite/c-c++-common/pr102281.c | 17 +++++++++++++ > >> .../gcc.target/aarch64/auto-init-6.c | 4 +-- > >> gcc/testsuite/gcc.target/i386/auto-init-2.c | 2 +- > >> gcc/testsuite/gcc.target/i386/auto-init-4.c | 10 +++----- > >> gcc/testsuite/gcc.target/i386/auto-init-6.c | 7 +++--- > >> 6 files changed, 47 insertions(+), 18 deletions(-) > >> create mode 100644 gcc/testsuite/c-c++-common/pr102281.c > >> > >> diff --git a/gcc/gimplify.c b/gcc/gimplify.c > >> index d8e4b139349..b27dc0ed308 100644 > >> --- a/gcc/gimplify.c > >> +++ b/gcc/gimplify.c > >> @@ -1784,8 +1784,8 @@ gimple_add_init_for_auto_var (tree decl, > >> that padding is initialized to zero. So, we always initialize paddings > >> to zeroes regardless INIT_TYPE. > >> To do the padding initialization, we insert a call to > >> - __BUILTIN_CLEAR_PADDING (&decl, 0, for_auto_init = true). > >> - Note, we add an additional dummy argument for __BUILTIN_CLEAR_PADDING, > >> + __builtin_clear_padding (&decl, 0, for_auto_init = true). > >> + Note, we add an additional dummy argument for __builtin_clear_padding, > >> 'for_auto_init' to distinguish whether this call is for automatic > >> variable initialization or not. > >> */ > >> @@ -1954,8 +1954,14 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p) > >> pattern initialization. > >> In order to make the paddings as zeroes for pattern init, We > >> should add a call to __builtin_clear_padding to clear the > >> - paddings to zero in compatiple with CLANG. */ > >> - if (flag_auto_var_init == AUTO_INIT_PATTERN) > >> + paddings to zero in compatiple with CLANG. > >> + We cannot insert this call if the variable is a gimple register > >> + since __builtin_clear_padding will take the address of the > >> + variable. As a result, if a long double/_Complex long double > >> + variable will spilled into stack later, its padding is 0XFE. */ > >> + if (flag_auto_var_init == AUTO_INIT_PATTERN > >> + && !is_gimple_reg (decl) > >> + && clear_padding_type_may_have_padding_p (TREE_TYPE (decl))) > >> gimple_add_padding_init_for_auto_var (decl, is_vla, seq_p); > >> } > >> } > >> @@ -5384,12 +5390,19 @@ gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, > >> > >> /* If the user requests to initialize automatic variables, we > >> should initialize paddings inside the variable. Add a call to > >> - __BUILTIN_CLEAR_PADDING (&object, 0, for_auto_init = true) to > >> + __builtin_clear_pading (&object, 0, for_auto_init = true) to > >> initialize paddings of object always to zero regardless of > >> INIT_TYPE. Note, we will not insert this call if the aggregate > >> variable has be completely cleared already or it's initialized > >> - with an empty constructor. */ > >> + with an empty constructor. We cannot insert this call if the > >> + variable is a gimple register since __builtin_clear_padding will take > >> + the address of the variable. As a result, if a long double/_Complex long > >> + double variable will be spilled into stack later, its padding cannot > >> + be cleared with __builtin_clear_padding. We should clear its padding > >> + when it is spilled into memory. */ > >> if (is_init_expr > >> + && !is_gimple_reg (object) > >> + && clear_padding_type_may_have_padding_p (type) > >> && ((AGGREGATE_TYPE_P (type) && !cleared && !is_empty_ctor) > >> || !AGGREGATE_TYPE_P (type)) > >> && is_var_need_auto_init (object)) > >> diff --git a/gcc/testsuite/c-c++-common/pr102281.c b/gcc/testsuite/c-c++-common/pr102281.c > >> new file mode 100644 > >> index 00000000000..a961451b5a7 > >> --- /dev/null > >> +++ b/gcc/testsuite/c-c++-common/pr102281.c > >> @@ -0,0 +1,17 @@ > >> +/* PR102281 */ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-ftrivial-auto-var-init=zero -Wno-psabi" } */ > >> +long long var1; > >> +float var2; > >> +typedef long long V __attribute__((__vector_size__(2 * sizeof(long long)))); > >> +typedef float W __attribute__((__vector_size__(4 * sizeof(float)))); > >> + > >> +V foo (void) > >> +{ > >> + return (V) {var1}; > >> +} > >> + > >> +W bar (void) > >> +{ > >> + return (W) {var2}; > >> +} > >> diff --git a/gcc/testsuite/gcc.target/aarch64/auto-init-6.c b/gcc/testsuite/gcc.target/aarch64/auto-init-6.c > >> index 27c16b33678..0456c66f496 100644 > >> --- a/gcc/testsuite/gcc.target/aarch64/auto-init-6.c > >> +++ b/gcc/testsuite/gcc.target/aarch64/auto-init-6.c > >> @@ -1,6 +1,6 @@ > >> /* Verify pattern initialization for complex type automatic variables. */ > >> /* { dg-do compile } */ > >> -/* { dg-options "-ftrivial-auto-var-init=pattern -fdump-rtl-expand" } */ > >> +/* { dg-options "-ftrivial-auto-var-init=pattern" } */ > >> > >> > >> _Complex long double result; > >> @@ -15,4 +15,4 @@ _Complex long double foo() > >> return result; > >> } > >> > >> -/* { dg-final { scan-rtl-dump-times "0xfffffffffffffffe\\\]\\\) repeated x16" 3 "expand" } } */ > >> +/* { dg-final { scan-assembler-times "word\t-16843010" 14 } } */ > >> diff --git a/gcc/testsuite/gcc.target/i386/auto-init-2.c b/gcc/testsuite/gcc.target/i386/auto-init-2.c > >> index e22930ae89b..0c59c62dacf 100644 > >> --- a/gcc/testsuite/gcc.target/i386/auto-init-2.c > >> +++ b/gcc/testsuite/gcc.target/i386/auto-init-2.c > >> @@ -29,7 +29,7 @@ void foo() > >> return; > >> } > >> > >> -/* { dg-final { scan-rtl-dump-times "0xfffffffffffffffe" 2 "expand" } } */ > >> +/* { dg-final { scan-rtl-dump-times "0xfffffffffffffffe" 1 "expand" } } */ > >> /* { dg-final { scan-rtl-dump-times "0xfffffffffffffefe" 1 "expand" } } */ > >> /* { dg-final { scan-rtl-dump-times "0xfffffffffefefefe" 2 "expand" { target lp64 } } } */ > >> /* { dg-final { scan-rtl-dump-times "0xfefefefefefefefe" 3 "expand" { target lp64 } } } */ > >> diff --git a/gcc/testsuite/gcc.target/i386/auto-init-4.c b/gcc/testsuite/gcc.target/i386/auto-init-4.c > >> index 7b46c74a073..1803dd45842 100644 > >> --- a/gcc/testsuite/gcc.target/i386/auto-init-4.c > >> +++ b/gcc/testsuite/gcc.target/i386/auto-init-4.c > >> @@ -1,6 +1,6 @@ > >> /* Verify pattern initialization for floating point type automatic variables. */ > >> /* { dg-do compile } */ > >> -/* { dg-options "-ftrivial-auto-var-init=pattern -fdump-rtl-expand -march=x86-64 -mtune=generic -msse" } */ > >> +/* { dg-options "-ftrivial-auto-var-init=pattern -march=x86-64 -mtune=generic -msse" } */ > >> > >> long double result; > >> > >> @@ -14,8 +14,6 @@ long double foo() > >> return result; > >> } > >> > >> -/* { dg-final { scan-rtl-dump-times "0xfffffffffefefefe" 1 "expand" { target { ! ia32 } } } } */ > >> -/* { dg-final { scan-rtl-dump-times "\\\[0xfefefefefefefefe\\\]" 1 "expand" { target { ! ia32 } } } } */ > >> -/* { dg-final { scan-rtl-dump-times "0xfffffffffffffffe\\\]\\\) repeated x16" 1 "expand" { target { ! ia32 } } } } */ > >> -/* { dg-final { scan-rtl-dump-times "0xfffffffffefefefe" 2 "expand" { target ia32 } } } */ > >> -/* { dg-final { scan-rtl-dump-times "\\\[0xfefefefefefefefe\\\]" 2 "expand" { target ia32 } } } */ > >> + > >> +/* { dg-final { scan-assembler-times "long\t-16843010" 5 { target { ! ia32 } } } } */ > >> +/* { dg-final { scan-assembler-times "long\t-16843010" 3 { target { ia32 } } } } */ > >> diff --git a/gcc/testsuite/gcc.target/i386/auto-init-6.c b/gcc/testsuite/gcc.target/i386/auto-init-6.c > >> index f75081edce4..339f8bc2966 100644 > >> --- a/gcc/testsuite/gcc.target/i386/auto-init-6.c > >> +++ b/gcc/testsuite/gcc.target/i386/auto-init-6.c > >> @@ -1,6 +1,6 @@ > >> /* Verify pattern initialization for complex type automatic variables. */ > >> /* { dg-do compile } */ > >> -/* { dg-options "-ftrivial-auto-var-init=pattern -fdump-rtl-expand -march=x86-64 -mtune=generic -msse" } */ > >> +/* { dg-options "-ftrivial-auto-var-init=pattern -march=x86-64 -mtune=generic -msse" } */ > >> > >> > >> _Complex long double result; > >> @@ -15,5 +15,6 @@ _Complex long double foo() > >> return result; > >> } > >> > >> -/* { dg-final { scan-rtl-dump-times "\\\[0xfefefefefefefefe\\\]" 1 "expand" } } */ > >> -/* { dg-final { scan-rtl-dump-times "0xfffffffffffffffe\\\]\\\) repeated x16" 2 "expand" } } */ > >> +/* { dg-final { scan-assembler-times "long\t-16843010" 10 { target { ! ia32 } } } } */ > >> +/* { dg-final { scan-assembler-times "long\t-16843010" 6 { target { ia32 } } } } */ > >> + > >> -- > >> 2.27.0 > >> > >> > > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)