Re: Make max_align_t respect _Float128 [version 2]

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Sep 5, 2016 at 7:06 PM, Joseph Myers <joseph@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  <joseph@codesourcery.com>
>
>         * ginclude/stddef.h (max_align_t) [__i386__]: Add __float128
>         element.
>
> gcc/c-family:
> 2016-09-05  Joseph Myers  <joseph@codesourcery.com>
>
>         * c-common.c (cxx_fundamental_alignment_p): Also consider
>         alignment of float128_type_node.
>
> gcc/testsuite:
> 2016-09-05  Joseph Myers  <joseph@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
> joseph@codesourcery.com