On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill <ja...@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.to...@gmail.com> wrote: >>> >>> On Thu, Jun 25, 2020 at 1:10 AM Richard Biener >>> <richard.guent...@gmail.com> wrote: >>>> >>>> On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey <skpg...@gmail.com> >wrote: >>>>> >>>>> 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 } >>>> >>>> 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. >> 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
