On Mon, Sep 5, 2016 at 7:06 PM, Joseph Myers <jos...@codesourcery.com> wrote:
> [Patch version 2 adds an update to cxx_fundamental_alignment_p.]
>
> The _FloatN, _FloatNx, _DecimalN and _DecimalNx types are specified in
> such a way that they are basic types, meaning that max_align_t must be
> at least as aligned as those types.
>
> On 32-bit x86, max_align_t is currently 8-byte aligned, but
> _Decimal128 and _Float128 are 16-byte aligned, so the alignment of
> max_align_t needs to increase to meet the standard requirements for
> these types.
As doubles on 32bit x86 are 4 byte aligned I suppose the choice could have
been to make those types 8 byte aligned to avoid changes of max_align_t?
(with the obvious eventual performance penalty of needing to do unaligned
loads/stores to xmm registers).
Richard.
> This patch implements such an increase. Because max_align_t needs to
> be usable for C++ as well as for C, <stddef.h> can't actually refer to
> _Float128, but needs to use __float128 (or some other notation for the
> type) instead. And since __float128 is architecture-specific, there
> isn't a preprocessor conditional that means "__float128 is available"
> (whereas one could test __FLT128_MANT_DIG__ to see if _Float128 is
> available; __SIZEOF_FLOAT128__ is available on x86 only). But I
> believe the only case that actually has an alignment problem here is
> 32-bit x86, and <stddef.h> already has lots of conditional specific to
> particular architectures or OSes, so this patch uses a conditional on
> __i386__; that also is the minimal change that ensures neither size
> nor alignment of max_align_t is changed in any case other than where
> it is necessary. If any other architectures turn out to have such an
> issue, it will show up as failures of one of the testcases added by
> this patch.
>
> Such an increase is of course an ABI change, but a reasonably safe
> one, in that max_align_t doesn't tend to appear in library interfaces
> (rather, it's something to use when writing allocators and similar
> code; most matches found on codesearch.debian.net look like copies of
> the gnulib stddef.h module rather than anything actually using
> max_align_t at all).
>
> cxx_fundamental_alignment_p has a corresponding change (adding
> _Float128 to the types considered).
>
> (I think glibc malloc alignment should also increase to 16-byte on
> 32-bit x86 so it works for allocating objects of these types, which is
> now straightforward given the fix made for 32-bit powerpc.)
>
> Bootstrapped with no regressions on x86_64-pc-linux-gnu, and
> spot-tested with -m32 that the new float128-align.c test now compiles
> where previously it didn't. OK to commit?
>
> gcc:
> 2016-09-05 Joseph Myers <jos...@codesourcery.com>
>
> * ginclude/stddef.h (max_align_t) [__i386__]: Add __float128
> element.
>
> gcc/c-family:
> 2016-09-05 Joseph Myers <jos...@codesourcery.com>
>
> * c-common.c (cxx_fundamental_alignment_p): Also consider
> alignment of float128_type_node.
>
> gcc/testsuite:
> 2016-09-05 Joseph Myers <jos...@codesourcery.com>
>
> * gcc.dg/float128-align.c, gcc.dg/float128x-align.c,
> gcc.dg/float16-align.c, gcc.dg/float32-align.c,
> gcc.dg/float32x-align.c, gcc.dg/float64-align.c,
> gcc.dg/float64x-align.c, gcc.dg/floatn-align.h: New tests.
>
> Index: gcc/c-family/c-common.c
> ===================================================================
> --- gcc/c-family/c-common.c (revision 239987)
> +++ gcc/c-family/c-common.c (working copy)
> @@ -12855,8 +12855,11 @@ scalar_to_vector (location_t loc, enum tree_code c
> bool
> cxx_fundamental_alignment_p (unsigned align)
> {
> - return (align <= MAX (TYPE_ALIGN (long_long_integer_type_node),
> - TYPE_ALIGN (long_double_type_node)));
> + unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
> + TYPE_ALIGN (long_double_type_node));
> + if (float128_type_node != NULL_TREE)
> + max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
> + return align <= max_align;
> }
>
> /* Return true if T is a pointer to a zero-sized aggregate. */
> Index: gcc/ginclude/stddef.h
> ===================================================================
> --- gcc/ginclude/stddef.h (revision 239987)
> +++ gcc/ginclude/stddef.h (working copy)
> @@ -426,6 +426,14 @@ typedef __WINT_TYPE__ wint_t;
> typedef struct {
> long long __max_align_ll __attribute__((__aligned__(__alignof__(long
> long))));
> long double __max_align_ld __attribute__((__aligned__(__alignof__(long
> double))));
> + /* _Float128 is defined as a basic type, so max_align_t must be
> + sufficiently aligned for it. This code must work in C++, so we
> + use __float128 here; that is only available on some
> + architectures, but only on i386 is extra alignment needed for
> + __float128. */
> +#ifdef __i386__
> + __float128 __max_align_f128
> __attribute__((__aligned__(__alignof(__float128))));
> +#endif
> } max_align_t;
> #endif
> #endif /* C11 or C++11. */
> Index: gcc/testsuite/gcc.dg/float128-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float128-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float128-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float128 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float128 } */
> +/* { dg-require-effective-target float128 } */
> +
> +#define WIDTH 128
> +#define EXT 0
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/float128x-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float128x-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float128x-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float128 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float128x } */
> +/* { dg-require-effective-target float128x } */
> +
> +#define WIDTH 128
> +#define EXT 1
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/float16-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float16-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float16-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float16 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float16 } */
> +/* { dg-require-effective-target float16 } */
> +
> +#define WIDTH 16
> +#define EXT 0
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/float32-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float32-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float32-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float32 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float32 } */
> +/* { dg-require-effective-target float32 } */
> +
> +#define WIDTH 32
> +#define EXT 0
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/float32x-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float32x-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float32x-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float32 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float32x } */
> +/* { dg-require-effective-target float32x } */
> +
> +#define WIDTH 32
> +#define EXT 1
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/float64-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float64-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float64-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float64 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float64 } */
> +/* { dg-require-effective-target float64 } */
> +
> +#define WIDTH 64
> +#define EXT 0
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/float64x-align.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/float64x-align.c (nonexistent)
> +++ gcc/testsuite/gcc.dg/float64x-align.c (working copy)
> @@ -0,0 +1,9 @@
> +/* Test _Float64 alignment. */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +/* { dg-add-options float64x } */
> +/* { dg-require-effective-target float64x } */
> +
> +#define WIDTH 64
> +#define EXT 1
> +#include "floatn-align.h"
> Index: gcc/testsuite/gcc.dg/floatn-align.h
> ===================================================================
> --- gcc/testsuite/gcc.dg/floatn-align.h (nonexistent)
> +++ gcc/testsuite/gcc.dg/floatn-align.h (working copy)
> @@ -0,0 +1,18 @@
> +/* Tests for _FloatN / _FloatNx types: test max_align_t alignment.
> + Before including this file, define WIDTH as the value N; define EXT
> + to 1 for _FloatNx and 0 for _FloatN. */
> +
> +#define CONCATX(X, Y) X ## Y
> +#define CONCAT(X, Y) CONCATX (X, Y)
> +#define CONCAT3(X, Y, Z) CONCAT (CONCAT (X, Y), Z)
> +
> +#if EXT
> +# define TYPE CONCAT3 (_Float, WIDTH, x)
> +#else
> +# define TYPE CONCAT (_Float, WIDTH)
> +#endif
> +
> +#include <stddef.h>
> +
> +_Static_assert (_Alignof (max_align_t) >= _Alignof (TYPE),
> + "max_align_t must be at least as aligned as _Float* types");
>
> --
> Joseph S. Myers
> jos...@codesourcery.com
