On Fri, Jun 14, 2013 at 08:18:19AM +0930, Alan Modra wrote:
> On Thu, Jun 13, 2013 at 05:42:17PM +0200, Jakub Jelinek wrote:
> > On Fri, Jun 14, 2013 at 01:07:01AM +0930, Alan Modra wrote:
> > > @@ -5774,10 +5818,11 @@ offsettable_ok_by_alignment (rtx op, HOST_WIDE_INT
> > > type = TREE_TYPE (decl);
> > >
> > > dalign = TYPE_ALIGN (type);
> > > + dalign = DATA_ABI_ALIGNMENT (type, dalign);
> > > if (CONSTANT_CLASS_P (decl))
> > > dalign = CONSTANT_ALIGNMENT (decl, dalign);
> > > else
> > > - dalign = DATA_ALIGNMENT (decl, dalign);
> > > + dalign = DATA_ALIGNMENT (type, dalign);
> > >
> > > if (dsize == 0)
> > > {
> >
> > What is this code trying to do? Shouldn't it just use DECL_ALIGN
> > which should be set to the right value from get_variable_alignment?
> > I mean, if !decl_binds_to_current_def_p (decl), then using DATA_ALIGNMENT
> > or CONSTANT_ALIGNMENT (for anything but actually emitting the var into
> > object, or just as an optimization hint that very likely the decl will be
> > aligned enough, but not guaranteed), which are optimization, is wrong
> > (an ABI problem).
>
> It is handling !DECL_P trees, which must be local. I know I saw
> STRING_CST here when I wrote offsettable_ok_by_alignment, hence the
> use of CONSTANT_ALIGNMENT. I'm not so sure about the need for
> DATA_ALIGNMENT now, but if it was correct before then we ought to
> be using both DATA_ABI_ALIGNMENT and DATA_ALIGNMENT after your
> changes.
Yeah, then it makes sense. Sorry for not looking up earlier that this is
the !DECL_P case.
As for the
typedef int vec_align __attribute__ ((vector_size(16), aligned(32)));
vec_align x = { 0, 0, 0, 0 };
changes, that is ABI changing bugfix, so the question is, are you fine with
breaking the ABI (between 4.8 and 4.9, or if you wanted to backport it to
4.8 too (I certainly plan to backport the non-ppc DATA_ABI_ALIGNMENT changes
to 4.8.2, already am using it in our compilers))? The other option is
to fix the ABI, but keep things backwards ABI compatible. That would be
done by decreasing the alignment as it used to do before in DATA_ABI_ALIGNMENT,
and increasing it to the desirable level only in DATA_ALIGNMENT. That has
the effect that when emitting the decls into assembly e.g. the above will
now be correctly 32 byte aligned, but accesses to such decl in compiler
generated code will only assume that alignment if
decl_binds_to_current_def_p, otherwise they will keep assuming the old
(broken) lowered alignment. At least for 4.8 backport IMHO that would be a
better idea (but of course would need big comment explaning it).
Jakub
