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?
/* { dg-require-effective-target ia32 } */
If all pr95237-*.c cases require target ia32, then shouldn't they be moved to
gcc.target/i386/ ?
Regards,
Dimitar
