Message ID | 20200827083542.GH2961@tucnak |
---|---|
State | New |
Headers | show |
Series | ia32: Fix alignment of _Atomic fields [PR65146] | expand |
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 >
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 > >
--- 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];