On Mon, 2 Nov 2020, Bernd Edlinger wrote:
> On 11/2/20 3:07 PM, Richard Biener wrote:
> > On Mon, 2 Nov 2020, Bernd Edlinger wrote:
> >
> >> Hi,
> >>
> >> this makes sure that stack allocated SSA_NAMEs are
> >> at least MODE_ALIGNED. Also increase the MEM_ALIGN
> >> for the corresponding rtl objects.
> >>
> >>
> >> Tested on x86_64-pc-linux-gnu and arm-none-eabi.
> >>
> >> OK for trunk?
> >
> >
> > @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base,
> > unsigned base_align,
> > }
> >
> > set_rtl (decl, x);
> > +
> > + if (TREE_CODE (decl) == SSA_NAME
> > + && GET_MODE (x) != BLKmode
> > + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x)))
> > + {
> > + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <=
> > base_align);
> > + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x)));
> > + }
> > }
> >
> >
> > I wonder whether we cannot "fix" set_rtl to not call
> > set_mem_attributes in this path, maybe directly call
> > set_mem_align there instead? That is, the preceeding
> > code for ! SSA_NAME already tries to adjust alignment
> > to honor that of the actual stack slot - IMHO the
> > non-SSA and SSA cases should be merged after this
> > patch, but maybe simply by calling set_mem_align
> > instead of doing the DECL_ALIGN frobbing and do
> > the alignment compute also for SSA_NAMEs?
> >
> > The other pieces look OK but the above is a bit ugly
> > at the moment.
> >
>
> Hmm, how about this?
That would work for me. Guess removing the DECL_ALIGN frobbing
in the != SSA_NAME path didn't work out or you didn't try out
of caution?
Richard.
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -1007,20 +1007,21 @@ expand_one_stack_var_at (tree decl, rtx base,
> unsigned base_align,
> x = plus_constant (Pmode, base, offset);
> x = gen_rtx_MEM (TREE_CODE (decl) == SSA_NAME
> ? TYPE_MODE (TREE_TYPE (decl))
> - : DECL_MODE (SSAVAR (decl)), x);
> + : DECL_MODE (decl), x);
> +
> + /* Set alignment we actually gave this decl if it isn't an SSA name.
> + If it is we generate stack slots only accidentally so it isn't as
> + important, we'll simply set the alignment directly on the MEM_P. */
> +
> + if (base == virtual_stack_vars_rtx)
> + offset -= frame_phase;
> + align = known_alignment (offset);
> + align *= BITS_PER_UNIT;
> + if (align == 0 || align > base_align)
> + align = base_align;
>
> if (TREE_CODE (decl) != SSA_NAME)
> {
> - /* Set alignment we actually gave this decl if it isn't an SSA name.
> - If it is we generate stack slots only accidentally so it isn't as
> - important, we'll simply use the alignment that is already set. */
> - if (base == virtual_stack_vars_rtx)
> - offset -= frame_phase;
> - align = known_alignment (offset);
> - align *= BITS_PER_UNIT;
> - if (align == 0 || align > base_align)
> - align = base_align;
> -
> /* One would think that we could assert that we're not decreasing
> alignment here, but (at least) the i386 port does exactly this
> via the MINIMUM_ALIGNMENT hook. */
> @@ -1031,13 +1032,7 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned
> base_align,
>
> set_rtl (decl, x);
>
> - if (TREE_CODE (decl) == SSA_NAME
> - && GET_MODE (x) != BLKmode
> - && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x)))
> - {
> - gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= base_align);
> - set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x)));
> - }
> + set_mem_align (x, align);
> }
>
> class stack_vars_data
>
>
> Is it OK if it passes bootstrap and regtesting ?
>
> Thanks
> Bernd.
>
> > Thanks,
> > Richard,
> >
> >>
> >>
> >> Thanks
> >> Bernd.
> >>
> >
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
