| Message ID | eea34e62-f0f5-11c5-7386-e80fd2686471@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Fix default alignment ABI break caused by MMA base support | expand |
Hi! On Fri, Nov 06, 2020 at 04:18:00PM -0600, Peter Bergner wrote: > As part of the MMA base support, we incremented BIGGEST_ALIGNMENT in > order to align the __vector_pair and __vector_quad types to 256 and 512 > bits respectively. This had the unintended effect of changing the > default alignment used by __attribute__ ((__aligned__)) which causes > an ABI break because of some dodgy code in GLIBC's struct pthread > (GLIBC is going to fix that too). The fix in GCC is to revert the > BIGGEST_ALIGNMENT change and to force the alignment on the type itself > rather than the mode used by the type. The ABI break is all GCC's faultt, but it is exposed by that glibc code, sure :-) > This passes bootstrap and regtesting on powerpc64le-linux with no regressions. > Ok for mainline and the GCC 10 release branch after some burn in? Yes, okay for both. Thanks! (See note below though.) > gcc/ > * config/rs6000/rs6000.h (BIGGEST_ALIGNMENT): Revert previous commit > so as not to break the ABI. > * config/rs6000/rs6000-call.c (rs6000_init_builtins): Set the ABI > mandated alignment for __vector_pair and __vector_quad types. > > gcc/testsuite/ > * gcc.target/powerpc/mma-alignment.c: New test. > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/mma-alignment.c > @@ -0,0 +1,41 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target hard_float } */ > +/* { dg-options "-O2 -mhard-float" } */ > + > +#include <stdlib.h> > + > +/* The MMA types below are enabled for pre-power10 compiles, because the > + built-ins that use them must always be initialized in case the user has > + a target attribute or pragma on a function that uses the MMA built-ins. > + Since the test below doesn't need any other MMA support, we can enable > + this test case on basically any cpu that has hard floating point > + registers. */ Good comment, thanks :-) That "basically" does not sound super convincing, but this is just a testcase, we can fix it later if need be. Segher
On 11/6/20 4:51 PM, Segher Boessenkool wrote: > The ABI break is all GCC's faultt, but it is exposed by that glibc code, > sure :-) Right, and I'm thankful it was caught (fairly) early enough! >> This passes bootstrap and regtesting on powerpc64le-linux with no regressions. >> Ok for mainline and the GCC 10 release branch after some burn in? > > Yes, okay for both. Thanks! Thanks, pushed to trunk. I'll push to GCC 10 in a day or two. >> + Since the test below doesn't need any other MMA support, we can enable >> + this test case on basically any cpu that has hard floating point >> + registers. */ > > Good comment, thanks :-) That "basically" does not sound super > convincing, but this is just a testcase, we can fix it later if need be. The types and built-ins are enabled with TARGET_EXTRA_BUILTINS which is a conglomeration of a lot of different target flags. The hard float test seemed like the test that would allow it to be run on the most targets, but being disabled when it should be. Yeah, we can fix it if we trip over it somehow. Peter
On Fri, Nov 06, 2020 at 04:18:00PM -0600, Peter Bergner via Gcc-patches wrote: > As part of the MMA base support, we incremented BIGGEST_ALIGNMENT in > order to align the __vector_pair and __vector_quad types to 256 and 512 > bits respectively. This had the unintended effect of changing the > default alignment used by __attribute__ ((__aligned__)) which causes > an ABI break because of some dodgy code in GLIBC's struct pthread > (GLIBC is going to fix that too). The fix in GCC is to revert the > BIGGEST_ALIGNMENT change and to force the alignment on the type itself > rather than the mode used by the type. [snip] > diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h > index bbd8060e143..5a47aa14722 100644 > --- a/gcc/config/rs6000/rs6000.h > +++ b/gcc/config/rs6000/rs6000.h > @@ -776,8 +776,10 @@ extern unsigned rs6000_pointer_size; > /* Allocation boundary (in *bits*) for the code of a function. */ > #define FUNCTION_BOUNDARY 32 > > -/* No data type wants to be aligned rounder than this. */ > -#define BIGGEST_ALIGNMENT (TARGET_MMA ? 512 : 128) > +/* No data type is required to be aligned rounder than this. Warning, if > + BIGGEST_ALIGNMENT is changed, then this may be an ABI break. An example > + of where this can break an ABI is in GLIBC's struct _Unwind_Exception. */ Instead of calling out something that is expected to be fixed, should you instead call out that it will change the alignment of anything using "__attribute__ ((__aligned__))"? > +#define BIGGEST_ALIGNMENT 128 [snip] PC
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h index bbd8060e143..5a47aa14722 100644 --- a/gcc/config/rs6000/rs6000.h +++ b/gcc/config/rs6000/rs6000.h @@ -776,8 +776,10 @@ extern unsigned rs6000_pointer_size; /* Allocation boundary (in *bits*) for the code of a function. */ #define FUNCTION_BOUNDARY 32 -/* No data type wants to be aligned rounder than this. */ -#define BIGGEST_ALIGNMENT (TARGET_MMA ? 512 : 128) +/* No data type is required to be aligned rounder than this. Warning, if + BIGGEST_ALIGNMENT is changed, then this may be an ABI break. An example + of where this can break an ABI is in GLIBC's struct _Unwind_Exception. */ +#define BIGGEST_ALIGNMENT 128 /* Alignment of field after `int : 0' in a structure. */ #define EMPTY_FIELD_BOUNDARY 32 diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 92378e958a9..3bd89a79bad 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -13191,12 +13191,14 @@ rs6000_init_builtins (void) if (TARGET_EXTRA_BUILTINS) { vector_pair_type_node = make_unsigned_type (256); + SET_TYPE_ALIGN (vector_pair_type_node, 256); SET_TYPE_MODE (vector_pair_type_node, POImode); layout_type (vector_pair_type_node); lang_hooks.types.register_builtin_type (vector_pair_type_node, "__vector_pair"); vector_quad_type_node = make_unsigned_type (512); + SET_TYPE_ALIGN (vector_quad_type_node, 512); SET_TYPE_MODE (vector_quad_type_node, PXImode); layout_type (vector_quad_type_node); lang_hooks.types.register_builtin_type (vector_quad_type_node, diff --git a/gcc/testsuite/gcc.target/powerpc/mma-alignment.c b/gcc/testsuite/gcc.target/powerpc/mma-alignment.c new file mode 100644 index 00000000000..09818931b48 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/mma-alignment.c @@ -0,0 +1,41 @@ +/* { dg-do run } */ +/* { dg-require-effective-target hard_float } */ +/* { dg-options "-O2 -mhard-float" } */ + +#include <stdlib.h> + +/* The MMA types below are enabled for pre-power10 compiles, because the + built-ins that use them must always be initialized in case the user has + a target attribute or pragma on a function that uses the MMA built-ins. + Since the test below doesn't need any other MMA support, we can enable + this test case on basically any cpu that has hard floating point + registers. */ + +struct +{ + int __attribute__ ((__aligned__)) ivar; + __vector_pair pair; + __vector_quad quad; +} s; + +int +main (void) +{ + /* Verify default alignment is 16-byte aligned (BIGGEST_ALIGNMENT). + This may change in the future, but that is an ABI break, so this + hardcoded test case is here to be a noisy FAIL as a warning, in + case the ABI change was unintended and unwanted. An example of where + this can break an ABI is in glibc's struct _Unwind_Exception. */ + if (__alignof__ (s.ivar) != 16) + abort (); + + /* Verify __vector_pair types are 32-byte aligned. */ + if (__alignof__ (s.pair) != 32) + abort (); + + /* Verify __vector_quad types are 64-byte aligned. */ + if (__alignof__ (s.quad) != 64) + abort (); + + return 0; +}