Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]

[Richard Biener <richard.guenther@gmail.com>]

On Sat, Jul 18, 2020 at 7:57 AM Sunil Pandey <skpgkp1@gmail.com> wrote:
>
> On Fri, Jul 17, 2020 at 1:22 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey <skpgkp1@gmail.com> wrote:
> > >
> > > Any comment on revised patch? At least,  in finish_decl, decl global attributes are populated.
> >
> > +static void
> > +ix86_lower_local_decl_alignment (tree decl)
> > +{
> > +  unsigned new_align = LOCAL_DECL_ALIGNMENT (decl);
> >
> > please use the macro-expanded call here since we want to amend
> > ix86_local_alignment to _not_ return a lower alignment when
> > called as LOCAL_DECL_ALIGNMENT (by adding a new parameter
> > to ix86_local_alignment).  Can you also amend the patch in this
> > way?
> >
> > +  if (new_align < DECL_ALIGN (decl))
> > +    SET_DECL_ALIGN (decl, new_align);
> >
> > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> > index 81bd2ee94f0..1ae99e30ed1 100644
> > --- a/gcc/c/c-decl.c
> > +++ b/gcc/c/c-decl.c
> > @@ -5601,6 +5601,8 @@ finish_decl (tree decl, location_t init_loc, tree init,
> >      }
> >
> >    invoke_plugin_callbacks (PLUGIN_FINISH_DECL, decl);
> > +  /* Lower local decl alignment.  */
> > +  lower_decl_alignment (decl);
> >  }
> >
> > should come before plugin hook invocation, likewise for the cp_finish_decl case.
> >
> > +/* Lower DECL alignment.  */
> > +
> > +void
> > +lower_decl_alignment (tree decl)
> > +{
> > +  if (VAR_P (decl)
> > +      && !is_global_var (decl)
> > +      && !DECL_HARD_REGISTER (decl))
> > +    targetm.lower_local_decl_alignment (decl);
> > +}
> >
> > please avoid this function, it's name sounds too generic and it's not worth
> > adding a public API for two calls.
> >
> > Alltogether this should avoid the x86 issue leaving left-overs (your identified
> > inliner case) as missed optimization [for the linux kernel which appearantly
> > decided that -mpreferred-stack-boundary=2 is a good ABI to use].
> >
> > Richard.
> >
> >
> Revised patch attached.

@@ -16776,7 +16783,7 @@ ix86_data_alignment (tree type, unsigned int
align, bool opt)

 unsigned int
 ix86_local_alignment (tree exp, machine_mode mode,
-                     unsigned int align)
+                     unsigned int align, bool setalign)
 {
   tree type, decl;

@@ -16801,6 +16808,10 @@ ix86_local_alignment (tree exp, machine_mode mode,
       && (!decl || !DECL_USER_ALIGN (decl)))
     align = 32;

+  /* Lower decl alignment.  */
+  if (setalign && align < DECL_ALIGN (decl))
+    SET_DECL_ALIGN (decl, align);
+
   /* If TYPE is NULL, we are allocating a stack slot for caller-save
      register in MODE.  We will return the largest alignment of XF
      and DF.  */

sorry for not being clear - the parameter should indicate whether an
alignment lower
than natural alignment is OK to return thus sth like

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 31757b044c8..19703cbceb9 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -16641,7 +16641,7 @@ ix86_data_alignment (tree type, unsigned int
align, bool opt)

 unsigned int
 ix86_local_alignment (tree exp, machine_mode mode,
-                     unsigned int align)
+                     unsigned int align, bool may_lower)
 {
   tree type, decl;

@@ -16658,7 +16658,8 @@ ix86_local_alignment (tree exp, machine_mode mode,

   /* Don't do dynamic stack realignment for long long objects with
      -mpreferred-stack-boundary=2.  */
-  if (!TARGET_64BIT
+  if (may_lower
+      && !TARGET_64BIT
       && align == 64
       && ix86_preferred_stack_boundary < 64
       && (mode == DImode || (type && TYPE_MODE (type) == DImode))

I also believe that spill_slot_alignment () should be able to get the
lower alignment
for long long but not get_stack_local_alignment () (both use
STACK_SLOT_ALIGNMENT).
Some uses of STACK_SLOT_ALIGNMENT also look fishy with respect to mem attributes
and alignment.

Otherwise the patch looks reasonable to salvage a misguided optimization for
a non-standard ABI.  If it is sufficient to make the people using that ABI happy
is of course another question.  I'd rather see them stop using it ...

That said, I'm hesitant to be the only one OKing this ugliness but I'd
immediately
OK a patch removing the questionable hunk from ix86_local_alignment ;)

Jakub, Jeff - any opinion?

Richard.

> > > On Tue, Jul 14, 2020 at 8:37 AM Sunil Pandey <skpgkp1@gmail.com> wrote:
> > >>
> > >> On Sat, Jul 4, 2020 at 9:11 AM Richard Biener
> > >> <richard.guenther@gmail.com> wrote:
> > >> >
> > >> > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
> > >> > >On 6/29/20 5:00 AM, Richard Biener wrote:
> > >> > >> On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >> > >>>
> > >> > >>> On Thu, Jun 25, 2020 at 1:10 AM Richard Biener
> > >> > >>> <richard.guenther@gmail.com> wrote:
> > >> > >>>>
> > >> > >>>> On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey <skpgkp1@gmail.com>
> > >> > >wrote:
> > >> > >>>>>
> > >> > >>>>> 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     }
> > >> > >>>>
> > >> > >>>> 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, &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?
> > >> > >>>>
> > >> > >>>
> > >> > >>> Here is the updated patch from Sunil.
> > >> > >>
> > >> > >> It does not address the fundamental issue that during
> > >> > >> do_type_align the is_global_var predicate is not
> > >> > >> reliable.  This means that for
> > >> > >>
> > >> > >> int main()
> > >> > >> {
> > >> > >>    extern long z;
> > >> > >> }
> > >> > >>
> > >> > >> the new hook (with -m32 -mpreferred-stack-boundary=2)
> > >> > >> will lower the alignment of 'z' which looks wrong.  During
> > >> > >> layout_decl we can unfortunately not distinguish between
> > >> > >> locals and globals.  We need to find another spot to adjust
> > >> > >> alignment of locals.  For C that might be in finish_decl,
> > >> > >> for C++ there's probably another suitable place.
> > >> > >
> > >> > >cp_finish_decl could work, but layout_decl seems like the right spot;
> > >> > >if
> > >> > >the problem is that the appropriate flags currently aren't being set in
> > >> > >
> > >> > >time, can't we fix that?
> > >> >
> > >> > The first and usually only call to layout_decl is from build_decl which gets nothing more than the type... But yes, I also initially thought that's the correct spot but it turns out it isn't.
> > >>
> > >> I added a new function lower_decl_alignment and invoked from
> > >> cp_decl_finish/decl_finish. Attached is revised patch.
> > >> >
> > >> > >> Note it needs to be a place before the frontends possibly
> > >> > >> inspect the alignment of the decl
> > >> > >> In C++ constexpr evalualtion might also expose alignment
> > >> > >> "early" so we really need a frontend solution here.
> > >> > >
> > >> > >Yes, we need to know the alignment right after the declaration.
> > >> > >
> > >> > >Jason
> > >> >