From: "H.J. Lu" <hjl.tools@gmail.com>
To: Uros Bizjak <ubizjak@gmail.com>
Cc: Jakub Jelinek <jakub@redhat.com>, Michael Matz <matz@suse.de>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] ia32: Fix alignment of _Atomic fields [PR65146]
Date: Thu, 27 Aug 2020 07:45:59 -0700 [thread overview]
Message-ID: <CAMe9rOrddbF7kp1gXPU3cKCVYO1NPQLbnfOu1Sk2CMg-2Nb+YA@mail.gmail.com> (raw)
In-Reply-To: <CAFULd4Y7PgkrR+gp5XPF+Gnw4DAk0+VEpfPsb7kW8AdVS_4YFg@mail.gmail.com>
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.
prev parent reply other threads:[~2020-08-27 14:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-27 8:35 Jakub Jelinek
2020-08-27 12:20 ` Uros Bizjak
2020-08-27 14:45 ` H.J. Lu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMe9rOrddbF7kp1gXPU3cKCVYO1NPQLbnfOu1Sk2CMg-2Nb+YA@mail.gmail.com \
--to=hjl.tools@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=matz@suse.de \
--cc=ubizjak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).