Re: [PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237]

2020-06-25 Thread Richard Biener via Gcc-patches
On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey  wrote:
>
> 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=0x77ffbb40, 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)
>   type  size 
> unit-size 
> align:64 warn_if_not_align:0 symtab:0 alias-set -1
> canonical-type 0x7fffea801888 precision:64 min  0x7fffea7e8fd8 -9223372036854775808> max  9223372036854775807>
> 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 }

But that doesn't change anything (obviously).  layout_decl
is called quite early, too early it looks like.

Now there doesn't seem to be any other good place where
we are sure to catch the decl before we evaluate things
like __alignof__

void __attribute__((noipa))
foo (__SIZE_TYPE__ align, long long *p)
{
  if ((__SIZE_TYPE__)p & (align-1))
__builtin_abort ();
}
int main()
{
  long long y;
  foo (_Alignof y, );
  return 0;
}

Joseph/Jason - do you have a good recommendation
how to deal with targets where natural alignment
is supposed to be lowered for optimization purposes?
(this case is for i?86 to avoid dynamic stack re-alignment
to align long long to 8 bytes with -mpreferred-stack-boundary=2)

I note that for -mincoming-stack-boundary=2 we do perform
dynamic stack re-alignment already.

I can't find a suitable existing target macro/hook for this,
but my gut feeling is that the default alignment should
instead be the lower one and instead the alignment for
globals should be raised as optimization?

Thanks,
Richard.

> >
> > 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 

Re: [PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237]

2020-06-24 Thread Sunil Pandey via Gcc-patches
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=0x77ffbb40, 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
> > +++ 

Re: [PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237]

2020-06-24 Thread Richard Biener via Gcc-patches
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
would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to be
renamed to INCREASE_LOCAL_DECL_ALIGNMENT).

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))

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.

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{(0x8000 * 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}}.
> 

[PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237]

2020-06-23 Thread Sunil K Pandey via Gcc-patches
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.

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)))
+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{(0x8000 * 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{(0x8000 * 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