From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by sourceware.org (Postfix) with ESMTPS id C7F98383E81A for ; Thu, 25 Jun 2020 00:53:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C7F98383E81A Received: by mail-wr1-x443.google.com with SMTP id b6so4054402wrs.11 for ; Wed, 24 Jun 2020 17:53:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=YD4ya1qB8g8qQGUzl1TWCIUzsk0wVDdXTF4qUBIDMvY=; b=Da+IxBJjlzci4A2r5XIdZVVcr7wRLmYqPjRsAcyFs34itb8uqYtpbTa0eeIU20zzz9 LKIRiYTIb3QLtrYGwP1LYt20pj7aO9FNWloXIfszG5grlQoxlgtcvTKtu3JpKCBqJ4q5 D4KkUfo6y/PjBlupKyQJH9RshV+yW0SEHOfq6SEDa7K8P0fSfzyLbDI0+948o+Uc8QSM ZdR4qBER1G+LXFzJOIhnfnMcaqiOiSXWtdP0qlcqT1ljdvQAIJ80DNDNq45ARuhX8R0G mcdqlQ/EmGB9AUuj8zTCxA1c+yDmv9X0XZLbpVc/gLNjOuHaW5H4+l65JWYrAqJ9j2WM 2OGg== X-Gm-Message-State: AOAM531UAEPexc03ZULgAmJJEgBDGb941u7PEcEl+jnbgjURKRB+nyIC upEIIumMu4obsgGy7KwavSTEUealCShKSWZz/UU= X-Google-Smtp-Source: ABdhPJwOm2U4S4m0VppNktQHt9TrazQJeY9jg1HeNUfgiQIWCtuIl75nFuBVZXD/7Y/f/hVbct9YKPdFmR6sTxxiS8Q= X-Received: by 2002:adf:efc6:: with SMTP id i6mr31767364wrp.303.1593046390644; Wed, 24 Jun 2020 17:53:10 -0700 (PDT) MIME-Version: 1.0 References: <20200623152917.1742147-1-skpgkp2@gmail.com> In-Reply-To: From: Sunil Pandey Date: Wed, 24 Jun 2020 17:52:59 -0700 Message-ID: Subject: Re: [PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237] To: Richard Biener Cc: Sunil K Pandey , "H. J. Lu" , GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, HK_RANDOM_ENVFROM, HK_RANDOM_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: Thu, 25 Jun 2020 00:53:14 -0000 On Wed, Jun 24, 2020 at 12:30 AM Richard Biener wrote: > > On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > wrote: > > > > From: Sunil K Pandey > > > > Default for this hook is NOP. For x86, in 32 bit mode, this hook > > sets alignment of long long on stack to 32 bits if preferred stack > > boundary is 32 bits. > > > > - This patch fixes > > gcc.target/i386/pr69454-2.c > > gcc.target/i386/stackalign/longlong-1.c > > - Regression test on x86-64, no new fail introduced. > > I think the name is badly chosen, TARGET_LOWER_LOCAL_DECL_ALIGNMENT Yes, I can change the target hook name. > would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to be > renamed to INCREASE_LOCAL_DECL_ALIGNMENT). It seems like LOCAL_DECL_ALIGNMENT macro documentation is incorrect. It increases as well as decreases alignment based on condition(-m32 -mpreferred-stack-boundary=2) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95885 > > You're calling it from do_type_align which IMHO is dangerous since that's > invoked from FIELD_DECL layout as well. Instead invoke it from > layout_decl itself where we do > > if (code != FIELD_DECL) > /* For non-fields, update the alignment from the type. */ > do_type_align (type, decl); > > and invoke the hook _after_ do_type_align. Also avoid > invoking the hook on globals or hard regs and only > invoke it on VAR_DECLs, thus only > > if (VAR_P (decl) && !is_global_var (decl) && !DECL_HARD_REGISTER (decl)) It seems like decl property is not fully populated at this point call to is_global_var (decl) on global variable return false. $ cat foo.c long long x; int main() { if (__alignof__(x) != 8) __builtin_abort(); return 0; } Breakpoint 1, layout_decl (decl=0x7ffff7ffbb40, known_align=0) at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout.c:674 674 do_type_align (type, decl); Missing separate debuginfos, use: dnf debuginfo-install gmp-6.1.2-10.fc31.x86_64 isl-0.16.1-9.fc31.x86_64 libmpc-1.1.0-4.fc31.x86_64 mpfr-3.1.6-5.fc31.x86_64 zlib-1.2.11-20.fc31.x86_64 (gdb) call debug_tree(decl) unit-size align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffea801888 precision:64 min max pointer_to_this > DI foo.c:1:11 size unit-size align:1 warn_if_not_align:0> (gdb) p is_global_var(decl) $1 = false (gdb) What about calling hook here 603 do_type_align (tree type, tree decl) 604 { 605 if (TYPE_ALIGN (type) > DECL_ALIGN (decl)) 606 { 607 SET_DECL_ALIGN (decl, TYPE_ALIGN (type)); 608 if (TREE_CODE (decl) == FIELD_DECL) 609 DECL_USER_ALIGN (decl) = TYPE_USER_ALIGN (type); 610 else 611 /* Lower local decl alignment */ 612 if (VAR_P (decl) 613 && !is_global_var (decl) 614 && !DECL_HARD_REGISTER (decl) 615 && cfun != NULL) 616 targetm.lower_local_decl_alignment (decl); 617 } > > Comments on the hook itself below. > > > Tested on x86-64. > > > > gcc/ChangeLog: > > > > PR target/95237 > > * config/i386/i386.c (ix86_update_decl_alignment): New > > function. > > (TARGET_UPDATE_DECL_ALIGNMENT): Define. > > * doc/tm.texi: Regenerate. > > * doc/tm.texi.in (TARGET_UPDATE_DECL_ALIGNMENT): New hook. > > * stor-layout.c (do_type_align): Call target hook to update > > decl alignment. > > * target.def (update_decl_alignment): New hook. > > > > gcc/testsuite/ChangeLog: > > > > PR target/95237 > > * gcc.target/i386/pr95237-1.c: New test. > > * gcc.target/i386/pr95237-2.c: New test. > > * gcc.target/i386/pr95237-3.c: New test. > > * gcc.target/i386/pr95237-4.c: New test. > > * gcc.target/i386/pr95237-5.c: New test. > > --- > > gcc/config/i386/i386.c | 22 ++++++++++++++++++++++ > > gcc/doc/tm.texi | 5 +++++ > > gcc/doc/tm.texi.in | 2 ++ > > gcc/stor-layout.c | 2 ++ > > gcc/target.def | 7 +++++++ > > gcc/testsuite/gcc.target/i386/pr95237-1.c | 16 ++++++++++++++++ > > gcc/testsuite/gcc.target/i386/pr95237-2.c | 10 ++++++++++ > > gcc/testsuite/gcc.target/i386/pr95237-3.c | 10 ++++++++++ > > gcc/testsuite/gcc.target/i386/pr95237-4.c | 10 ++++++++++ > > gcc/testsuite/gcc.target/i386/pr95237-5.c | 16 ++++++++++++++++ > > 10 files changed, 100 insertions(+) > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-1.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-2.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-3.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-4.c > > create mode 100644 gcc/testsuite/gcc.target/i386/pr95237-5.c > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > index 37aaa49996d..bcd9abd5303 100644 > > --- a/gcc/config/i386/i386.c > > +++ b/gcc/config/i386/i386.c > > @@ -16917,6 +16917,25 @@ ix86_minimum_alignment (tree exp, machine_mode mode, > > > > return align; > > } > > + > > +/* Implement TARGET_UPDATE_DECL_ALIGNMENT. */ > > + > > +static void > > +ix86_update_decl_alignment (tree decl) > > +{ > > + tree type = TREE_TYPE (decl); > > + > > + if (cfun != NULL > > + && !TARGET_64BIT > > + && DECL_ALIGN (decl) == 64 > > + && ix86_preferred_stack_boundary < 64 > > + && !is_global_var (decl) > > + && (DECL_MODE (decl) == E_DImode > > + || (type && TYPE_MODE (type) == E_DImode)) > > + && (!type || !TYPE_USER_ALIGN (type)) > > + && (!decl || !DECL_USER_ALIGN (decl))) > > I'd simply do > > unsigned new_align = LOCAL_DECL_ALIGNMENT (decl); > if (new_align < DECL_ALIGN (decl)) > SET_DECL_ALIGN (decl, new_align); > > to avoid spreading the logic to multiple places. Sure, I will incorporate this. > > Thanks, > Richard. > > > + SET_DECL_ALIGN (decl, 32); > > +} > > > > /* Find a location for the static chain incoming to a nested function. > > This is a register, unless all free registers are used by arguments. */ > > @@ -23519,6 +23538,9 @@ ix86_run_selftests (void) > > #undef TARGET_CAN_CHANGE_MODE_CLASS > > #define TARGET_CAN_CHANGE_MODE_CLASS ix86_can_change_mode_class > > > > +#undef TARGET_UPDATE_DECL_ALIGNMENT > > +#define TARGET_UPDATE_DECL_ALIGNMENT ix86_update_decl_alignment > > + > > #undef TARGET_STATIC_RTX_ALIGNMENT > > #define TARGET_STATIC_RTX_ALIGNMENT ix86_static_rtx_alignment > > #undef TARGET_CONSTANT_ALIGNMENT > > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi > > index 6e7d9dc54a9..c11ef5dca89 100644 > > --- a/gcc/doc/tm.texi > > +++ b/gcc/doc/tm.texi > > @@ -1086,6 +1086,11 @@ On 32-bit ELF the largest supported section alignment in bits is > > @samp{(0x80000000 * 8)}, but this is not representable on 32-bit hosts. > > @end defmac > > > > +@deftypefn {Target Hook} void TARGET_UPDATE_DECL_ALIGNMENT (tree @var{decl}) > > +Define this hook to update alignment of decl > > +@samp{(@var{decl}}. > > +@end deftypefn > > + > > @deftypefn {Target Hook} HOST_WIDE_INT TARGET_STATIC_RTX_ALIGNMENT (machine_mode @var{mode}) > > This hook returns the preferred alignment in bits for a > > statically-allocated rtx, such as a constant pool entry. @var{mode} > > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in > > index 3be984bbd5c..618acd73a1e 100644 > > --- a/gcc/doc/tm.texi.in > > +++ b/gcc/doc/tm.texi.in > > @@ -1036,6 +1036,8 @@ On 32-bit ELF the largest supported section alignment in bits is > > @samp{(0x80000000 * 8)}, but this is not representable on 32-bit hosts. > > @end defmac > > > > +@hook TARGET_UPDATE_DECL_ALIGNMENT > > + > > @hook TARGET_STATIC_RTX_ALIGNMENT > > > > @defmac DATA_ALIGNMENT (@var{type}, @var{basic-align}) > > diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c > > index bde6fa22b58..0687a68ba29 100644 > > --- a/gcc/stor-layout.c > > +++ b/gcc/stor-layout.c > > @@ -605,6 +605,8 @@ do_type_align (tree type, tree decl) > > if (TYPE_ALIGN (type) > DECL_ALIGN (decl)) > > { > > SET_DECL_ALIGN (decl, TYPE_ALIGN (type)); > > + /* Update decl alignment */ > > + targetm.update_decl_alignment (decl); > > if (TREE_CODE (decl) == FIELD_DECL) > > DECL_USER_ALIGN (decl) = TYPE_USER_ALIGN (type); > > } > > diff --git a/gcc/target.def b/gcc/target.def > > index 07059a87caf..e1695753470 100644 > > --- a/gcc/target.def > > +++ b/gcc/target.def > > @@ -3348,6 +3348,13 @@ HOOK_VECTOR_END (addr_space) > > #undef HOOK_PREFIX > > #define HOOK_PREFIX "TARGET_" > > > > +DEFHOOK > > +(update_decl_alignment, > > + "Define this hook to update alignment of decl\n\ > > +@samp{(@var{decl}}.", > > + void, (tree decl), > > + hook_void_tree) > > + > > DEFHOOK > > (static_rtx_alignment, > > "This hook returns the preferred alignment in bits for a\n\ > > diff --git a/gcc/testsuite/gcc.target/i386/pr95237-1.c b/gcc/testsuite/gcc.target/i386/pr95237-1.c > > new file mode 100644 > > index 00000000000..bc8a84ee0db > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr95237-1.c > > @@ -0,0 +1,16 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target ia32 } */ > > +/* { dg-options "-mpreferred-stack-boundary=2" } */ > > +typedef __UINTPTR_TYPE__ uintptr_t; > > +void __attribute__((noipa)) foo (long long *p, uintptr_t a) > > +{ > > + if ((uintptr_t)p & (a-1)) > > + __builtin_abort (); > > +} > > +int main() > > +{ > > + long long x; > > + uintptr_t a = __alignof__(x); > > + foo(&x, a); > > + return 0; > > +} > > diff --git a/gcc/testsuite/gcc.target/i386/pr95237-2.c b/gcc/testsuite/gcc.target/i386/pr95237-2.c > > new file mode 100644 > > index 00000000000..82ff777669a > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr95237-2.c > > @@ -0,0 +1,10 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target ia32 } */ > > +/* { dg-options "-mpreferred-stack-boundary=2" } */ > > +long long x; > > +int main() > > +{ > > + if (__alignof__(x) != 8) > > + __builtin_abort(); > > + return 0; > > +} > > diff --git a/gcc/testsuite/gcc.target/i386/pr95237-3.c b/gcc/testsuite/gcc.target/i386/pr95237-3.c > > new file mode 100644 > > index 00000000000..2fb1f630362 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr95237-3.c > > @@ -0,0 +1,10 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target ia32 } */ > > +/* { dg-options "-mpreferred-stack-boundary=2" } */ > > +int main() > > +{ > > + long long x; > > + if (__alignof__(x) != 4) > > + __builtin_abort(); > > + return 0; > > +} > > diff --git a/gcc/testsuite/gcc.target/i386/pr95237-4.c b/gcc/testsuite/gcc.target/i386/pr95237-4.c > > new file mode 100644 > > index 00000000000..d52a770d703 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr95237-4.c > > @@ -0,0 +1,10 @@ > > +/* { dg-do run } */ > > +/* { dg-require-effective-target ia32 } */ > > +/* { dg-options "-mpreferred-stack-boundary=4" } */ > > +int main() > > +{ > > + long long x; > > + if (__alignof__(x) != 8) > > + __builtin_abort(); > > + return 0; > > +} > > diff --git a/gcc/testsuite/gcc.target/i386/pr95237-5.c b/gcc/testsuite/gcc.target/i386/pr95237-5.c > > new file mode 100644 > > index 00000000000..4d9be06a045 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr95237-5.c > > @@ -0,0 +1,16 @@ > > +/* { dg-do compile { target ia32 } } */ > > +/* { dg-options "-mpreferred-stack-boundary=2 -Os -w" } */ > > + > > +int a; > > + > > +long long > > +b (void) > > +{ > > +} > > + > > +void > > +c (void) > > +{ > > + if (b()) > > + a = 1; > > +} > > -- > > 2.25.4 > >