[PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
Sunil Pandey
skpgkp1@gmail.com
Fri Jul 17 05:15:37 GMT 2020
Any comment on revised patch? At least, in finish_decl, decl global
attributes are populated.
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
> >
>
More information about the Gcc-patches
mailing list