On Wed, Jun 24, 2020 at 12:30 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > From: Sunil K Pandey <skpg...@gmail.com> > > > > 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) <var_decl 0x7ffff7ffbb40 x type <integer_type 0x7fffea801888 long long int DI size <integer_cst 0x7fffea7e8d38 constant 64> unit-size <integer_cst 0x7fffea7e8d50 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffea801888 precision:64 min <integer_cst 0x7fffea7e8fd8 -9223372036854775808> max <integer_cst 0x7fffea806000 9223372036854775807> pointer_to_this <pointer_type 0x7fffea8110a8>> DI foo.c:1:11 size <integer_cst 0x7fffea7e8d38 64> unit-size <integer_cst 0x7fffea7e8d50 8> 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 > >
