On Wed, Jul 22, 2020 at 7:25 AM Dimitar Dimitrov <dimi...@dinux.eu> wrote:
>
> On сряда, 22 юли 2020 г. 2:04:35 EEST Sunil Pandey via Gcc-patches wrote:
> > On Tue, Jul 21, 2020 at 12:50 AM Richard Biener
> >
> > <richard.guent...@gmail.com> wrote:
> > > On Tue, Jul 21, 2020 at 7:16 AM Sunil Pandey <skpg...@gmail.com> wrote:
> > > > On Mon, Jul 20, 2020 at 5:06 AM Richard Biener
> > > >
> > > > <richard.guent...@gmail.com> wrote:
> > > > > On Sat, Jul 18, 2020 at 7:57 AM Sunil Pandey <skpg...@gmail.com>
> wrote:
> > > > > > On Fri, Jul 17, 2020 at 1:22 AM Richard Biener
> > > > > >
> > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > > On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey <skpg...@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.
> > > >
> > > > Revised patch attached.
> > >
> > > You are now passing 'true' to ix86_local_alignment for all callers,
> > > that's not correct.
> > > I said at most STACK_SLOT_ALIGNMENT _might_ be able to take the lower
> > > alignment but some callers look suspicious so I wasn't sure.
> > >
> > > Which means - please pass false for LOCAL_ALIGNMENT, STACK_SLOT_ALIGNMENT
> > > and LOCAL_DECL_ALIGNMENT.
> > >
> > > + /* Lower local decl alignment. */
> > > +
> > > + if (VAR_P (decl)
> > > + && !is_global_var (decl)
> > > + && !DECL_HARD_REGISTER (decl))
> > > + targetm.lower_local_decl_alignment (decl);
> > >
> > > the comment is quite useless, just repeating what the code does.
> > > Please rephrase it
> > > as
> > >
> > > /* This is the last point we can lower alignment so give the target the
> > >
> > > chance to do so. */
> > >
> > > and remove the vertical space after the comment.
> > >
> > > OK with those changes.
> > > Richard.
> >
> > Updated patch attached. I will ask H.J. to check it in for me.
> > --Sunil
>
> Hi Sunil,
>
> I get regressions for PRU and AVR:
> NA->UNRESOLVED: c-c++-common/pr95237-6.c -std=gnu++14 execution test
>
> Is pr95237-6.c missing an ia32 target filter?
I am checking in this to limit it to x86 targets as an obvious fix.
> /* { dg-require-effective-target ia32 } */
>
> If all pr95237-*.c cases require target ia32, then shouldn't they be moved to
> gcc.target/i386/ ?
>
They are both C and C++ tests. gcc.target/i386/ is C only.
--
H.J.
From 265c6a41b78599309a8649d61879f69b4aae091d Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.to...@gmail.com>
Date: Wed, 22 Jul 2020 07:33:54 -0700
Subject: [PATCH] Limit pr95237-6.c to x86 targets
Since c-c++-common/pr95237-6.c is x86 specific, limit it to x86 targets.
PR target/95237
* c-c++-common/pr95237-6.c: Only run for x86 targets.
---
gcc/testsuite/c-c++-common/pr95237-6.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/testsuite/c-c++-common/pr95237-6.c b/gcc/testsuite/c-c++-common/pr95237-6.c
index ce1568fa282..5e7396d7f4e 100644
--- a/gcc/testsuite/c-c++-common/pr95237-6.c
+++ b/gcc/testsuite/c-c++-common/pr95237-6.c
@@ -1,5 +1,5 @@
-/* { dg-do run } */
-/* { dg-options "-O2" { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-do run { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-options "-O2" } */
#include <stddef.h>
#ifdef __x86_64__
# define EXP_ALIGN 8
--
2.26.2
