[PATCH] ia32: Fix alignment of _Atomic fields [PR65146]

[Uros Bizjak]

On Thu, Aug 27, 2020 at 10:35 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> For _Atomic fields, lowering the alignment of long long or double etc.
> fields on ia32 is undesirable, because then one really can't perform atomic
> operations on those using cmpxchg8b.
>
> The following patch stops lowering the alignment in fields for _Atomic
> types (the x86_field_alignment change) and for -mpreferred-stack-boundary=2
> also ensures we don't misalign _Atomic long long etc. automatic variables
> (the ix86_{local,minimum}_alignment changes).
> Not sure about iamcu_alignment change, I know next to nothing about IA MCU,
> but unless it doesn't have cmpxchg8b instruction, it would surprise me if we
> don't want to do it as well.
> clang apparently doesn't lower the field alignment for _Atomic.

Intel MCU implements pentium ISA, which includes cmpxchg8b.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2020-08-27  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/65146
>         * config/i386/i386.c (iamcu_alignment): Don't decrease alignment
>         for TYPE_ATOMIC types.
>         (ix86_local_alignment): Likewise.
>         (ix86_minimum_alignment): Likewise.
>         (x86_field_alignment): Likewise, and emit a -Wpsabi diagnostic
>         for it.
>
>         * gcc.target/i386/pr65146.c: New test.

LGTM, but please allow some time for HJ to comment.

Thanks,
Uros.

> --- gcc/config/i386/i386.c.jj   2020-08-24 10:00:01.321258451 +0200
> +++ gcc/config/i386/i386.c      2020-08-26 19:19:11.834297395 +0200
> @@ -16487,7 +16487,11 @@ iamcu_alignment (tree type, int align)
>
>    /* Intel MCU psABI specifies scalar types > 4 bytes aligned to 4
>       bytes.  */
> -  mode = TYPE_MODE (strip_array_types (type));
> +  type = strip_array_types (type);
> +  if (TYPE_ATOMIC (type))
> +    return align;
> +
> +  mode = TYPE_MODE (type);
>    switch (GET_MODE_CLASS (mode))
>      {
>      case MODE_INT:
> @@ -16644,7 +16648,8 @@ ix86_local_alignment (tree exp, machine_
>        && align == 64
>        && ix86_preferred_stack_boundary < 64
>        && (mode == DImode || (type && TYPE_MODE (type) == DImode))
> -      && (!type || !TYPE_USER_ALIGN (type))
> +      && (!type || (!TYPE_USER_ALIGN (type)
> +                   && !TYPE_ATOMIC (strip_array_types (type))))
>        && (!decl || !DECL_USER_ALIGN (decl)))
>      align = 32;
>
> @@ -16757,7 +16762,8 @@ ix86_minimum_alignment (tree exp, machin
>    /* Don't do dynamic stack realignment for long long objects with
>       -mpreferred-stack-boundary=2.  */
>    if ((mode == DImode || (type && TYPE_MODE (type) == DImode))
> -      && (!type || !TYPE_USER_ALIGN (type))
> +      && (!type || (!TYPE_USER_ALIGN (type)
> +                   && !TYPE_ATOMIC (strip_array_types (type))))
>        && (!decl || !DECL_USER_ALIGN (decl)))
>      {
>        gcc_checking_assert (!TARGET_STV);
> @@ -20293,11 +20299,30 @@ x86_field_alignment (tree type, int comp
>      return computed;
>    if (TARGET_IAMCU)
>      return iamcu_alignment (type, computed);
> -  mode = TYPE_MODE (strip_array_types (type));
> +  type = strip_array_types (type);
> +  mode = TYPE_MODE (type);
>    if (mode == DFmode || mode == DCmode
>        || GET_MODE_CLASS (mode) == MODE_INT
>        || GET_MODE_CLASS (mode) == MODE_COMPLEX_INT)
> -    return MIN (32, computed);
> +    {
> +      if (TYPE_ATOMIC (type) && computed > 32)
> +       {
> +         static bool warned;
> +
> +         if (!warned && warn_psabi)
> +           {
> +             const char *url
> +               = CHANGES_ROOT_URL "gcc-11/changes.html#ia32_atomic";
> +
> +             warned = true;
> +             inform (input_location, "the alignment of %<_Atomic %T%> "
> +                                     "fields changed in %{GCC 11.1%}",
> +                     TYPE_MAIN_VARIANT (type), url);
> +           }
> +       }
> +      else
> +      return MIN (32, computed);
> +    }
>    return computed;
>  }
>
> --- gcc/testsuite/gcc.target/i386/pr65146.c.jj  2020-08-26 19:25:27.720023020 +0200
> +++ gcc/testsuite/gcc.target/i386/pr65146.c     2020-08-26 19:28:24.982535651 +0200
> @@ -0,0 +1,12 @@
> +/* PR target/65146 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wno-psabi" } */
> +
> +struct A { char a; _Atomic long long b; };
> +struct B { char a; _Atomic double b; };
> +struct C { char a; _Atomic long long b[2]; };
> +struct D { char a; _Atomic double b[2]; };
> +extern int a[__builtin_offsetof (struct A, b) == 8 ? 1 : -1];
> +extern int b[__builtin_offsetof (struct B, b) == 8 ? 1 : -1];
> +extern int c[__builtin_offsetof (struct C, b) == 8 ? 1 : -1];
> +extern int d[__builtin_offsetof (struct D, b) == 8 ? 1 : -1];
>
>         Jakub
>