[PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]

[Jason Merrill]

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?

> 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