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. Note
it needs to be a place before the frontends possibly
inspect the alignment of the decl - I hope there's no
self-reflective way of using it like
void foo()
{
long a[__alignof__(this)];
}
with 'this' refering to the actual declarator. But you
never know C++ ...
In C++ constexpr evalualtion might also expose alignment
"early" so we really need a frontend solution here. My
limited C++ fu would come up with sth like
constexpr int a = []() { long x; return __alignof__(x); };
or so. I guess even local templates might expose alignment?
void foo()
{
long a;
template <int> struct X;
template <> struct X<4> {};
X<__alignof__(a)> b;
}
and eventually the alignof might be even dependent.
Richard.
> --
> H.J.
