* [PATCH] ia32: Fix alignment of _Atomic fields [PR65146]
@ 2020-08-27 8:35 Jakub Jelinek
2020-08-27 12:20 ` Uros Bizjak
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2020-08-27 8:35 UTC (permalink / raw)
To: Uros Bizjak, hjl.tools, Michael Matz; +Cc: gcc-patches
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.
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.
--- 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ia32: Fix alignment of _Atomic fields [PR65146]
2020-08-27 8:35 [PATCH] ia32: Fix alignment of _Atomic fields [PR65146] Jakub Jelinek
@ 2020-08-27 12:20 ` Uros Bizjak
2020-08-27 14:45 ` H.J. Lu
0 siblings, 1 reply; 3+ messages in thread
From: Uros Bizjak @ 2020-08-27 12:20 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: H. J. Lu, Michael Matz, gcc-patches
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ia32: Fix alignment of _Atomic fields [PR65146]
2020-08-27 12:20 ` Uros Bizjak
@ 2020-08-27 14:45 ` H.J. Lu
0 siblings, 0 replies; 3+ messages in thread
From: H.J. Lu @ 2020-08-27 14:45 UTC (permalink / raw)
To: Uros Bizjak; +Cc: Jakub Jelinek, Michael Matz, gcc-patches
On Thu, Aug 27, 2020 at 5:20 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> 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.
That is correct.
> > 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.
LGTM too. Thanks.
> 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
> >
--
H.J.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-27 14:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 8:35 [PATCH] ia32: Fix alignment of _Atomic fields [PR65146] Jakub Jelinek
2020-08-27 12:20 ` Uros Bizjak
2020-08-27 14:45 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).