Re: [PATCH] DATA_ALIGNMENT vs. DATA_ABI_ALIGNMENT (PR target/56564)

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Jun 07, 2013 at 01:43:27PM -0700, Richard Henderson wrote:
> On 06/07/2013 12:25 PM, Jakub Jelinek wrote:
> > This PR is about DATA_ALIGNMENT macro increasing alignment of some decls
> > for optimization purposes beyond ABI mandated levels.  It is fine to emit
> > the vars aligned as much as we want for optimization purposes, but if we
> > can't be sure that references to that decl bind to the definition we
> > increased the alignment on (e.g. common variables, or -fpic code without
> > hidden visibility, weak vars etc.), we can't assume that alignment.
> 
> When the linker merges common blocks, it chooses both maximum size and maximum
> alignment.  Thus for any common block for which we can prove the block must
> reside in the module (any executable, or hidden common in shared object), we
> can go ahead and use the increased alignment.

But consider say:
one TU:
struct S { char buf[15]; } s __attribute__((aligned (32)));

another TU:
char c = 7;
struct S { char buf[15]; } s = { { 1, 2 } };
char d = 8;
int main () { return 0; }

(the aligned(32) is there just to simulate the DATA_ALIGNMENT optimization
increase).  Linker warns about this (thus the question is if we want to
increase the alignment for optimization on commons at all) and doesn't align
it.

> This structure would seem to do the wrong thing if DATA_ABI_ALIGNMENT is
> defined, but DATA_ALIGNMENT isn't.  And while I realize you documented it, I
> don't like the restriction that D_A /must/ return something larger than D_A_A.
>  All that means is that in complex cases D_A will have to call D_A_A itself.

Yeah, I guess I can rearrange it.  The reason I wrote it that way was to
avoid an extra function call, but that is probably not big enough overhead.

> I would think that it would be better to rearrange as
> 
>   if (!D_U_A)
>     {
>   #ifdef D_A_A
>       align = ...
>   #endif
>   #ifdef D_A
>       if (d_b_t_c_d_p)
>         align = ...
>   #endif
>     }
> 
> Why the special case for TLS?  If we really want that special case surely that
> test should go into D_A_A itself, and not here in generic code.

I special case TLS for backwards ABI compatibility with GCC <= 4.8.1,
because we ignored DATA_ALIGNMENT for TLS vars, even if what it returned
wasn't just an optimization, but ABI requirement.  So, if you have:
__thread char a[16] = { 1, 2 };
in one shared library and
__thread char a[16] = { 1, 2 };
use (a);
in another shared library, compile one with GCC 4.8.1 e.g. and the other one
with 4.9.0, if use (a) would assume the psABI mandated 16 byte alignment,
but the actual definition was from 4.8.1 compiled object and was only 1 byte
aligned, it could crash.  Also, neither DATA_ALIGNMENT nor DATA_ABI_ALIGNMENT sees
the decl itself, it just looks at type and previously assumed alignment.

> > Bootstrapped/regtested on x86_64-linux and i686-linux.  No idea about other
> > targets, I've kept them all using DATA_ALIGNMENT, which is considered
> > optimization increase only now, if there is some ABI mandated alignment
> > increase on other targets, that should be done in DATA_ABI_ALIGNMENT as
> > well as DATA_ALIGNMENT.
> 
> I've had a brief look over the instances of D_A within the tree atm.  Most of
> them carry the cut-n-paste comment "for the same reasons".  These I believe
> never intended an ABI change, and were really only interested in optimization.

Thanks for looking into this.

> But these I think require a good hard look to see if they really intended an
> ABI alignment:
> 
> c6x	comment explicitly mentions abi
> cris	compiler options for alignment -- systemwide or local?
> mmix	comment mentions GETA instruction
> s390	comment mentions LARL instruction
> rs6000	SPE and E500 portion of the alignment non-optional?
> 
> Relevant port maintainers CCed.

	Jakub