[PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237]

Sunil Pandey skpgkp1@gmail.com
Thu Jun 25 00:52:59 GMT 2020


On Wed, Jun 24, 2020 at 12:30 AM Richard Biener
<richard.guenther@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 <skpgkp1@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
> >


More information about the Gcc-patches mailing list