[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #40 from H.J. Lu --- (In reply to Uroš Bizjak from comment #39) > Please open a new bugreport, failures in Comment 37 and Comment 38 have > nothing with r7-1112-gbeed3701c796842abbfb27d7484b35bd82818740 which was > fully reverted. > > (FTR, fixed peepholes were ineffective, so there was no loss in removing > them.) I opened PR 101761.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #39 from Uroš Bizjak --- Please open a new bugreport, failures in Comment 37 and Comment 38 have nothing with r7-1112-gbeed3701c796842abbfb27d7484b35bd82818740 which was fully reverted. (FTR, fixed peepholes were ineffective, so there was no loss in removing them.)
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182
--- Comment #38 from H.J. Lu ---
This time is 29_atomics/atomic_ref/wait_notify.cc in 64-bit on Skylake server:
(gdb) bt
#0 0x7f897288cc1d in syscall () from /lib64/libc.so.6
#1 0x004018be in std::__detail::__platform_wait (
__addr=__addr@entry=0x405400
,
__val=__val@entry=3)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:104
#2 0x004022a3 in std::__detail::__waiter_pool::_M_do_wait (__old=3,
__addr=0x405400 , this=)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:261
#3 std::__detail::__waiter
>::_M_do_wait_v(void* const*,
std::remove_volatile::type, std::memory_order)::{lambda()#1}>(void*,
std::__atomic_impl::wait(void* const*,
std::remove_volatile::type, std::memory_order)::{lambda()#1})
(__vfn=..., __old=0x2a, this=)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:400
#4 std::__atomic_wait_address_v(void*
const*, std::remove_volatile::type,
std::memory_order)::{lambda()#1}>(void* const*, void*,
std::__atomic_impl::wait(void* const*,
std::remove_volatile::type, std::memory_order)::{lambda()#1})
(__addr=,
__old=0x2a, __vfn=...)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_wait.h:430
#5 0x00402366 in std::__atomic_impl::wait (
__m=std::memory_order::seq_cst, __old=,
__ptr=)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_base.h:1018
#6 std::__atomic_ref::wait (
__m=std::memory_order::seq_cst, __old=, this=0x7ffed19864e8)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/atomic_base.h:1874
#7 test (va=va@entry=0x0, vb=vb@entry=0x2a)
at
/export/gnu/import/git/gitlab/x86-gcc/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:44
#8 0x0040125b in main ()
at
/export/gnu/import/git/gitlab/x86-gcc/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:57
(gdb)
GCC was configured with
--with-arch=native --with-cpu=native --prefix=/usr/12.0.0 --enable-clocale=gnu
--with-system-zlib --enable-shared --enable-cet --with-demangler-in-ld
--enable-libmpx --with-multilib-list=m32,m64,mx32 --with-fpmath=sse
This is very rare.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182
--- Comment #37 from H.J. Lu ---
I still see 32-bit test hang at random on Skylake server:
(gdb) bt
#0 0xf7fc655d in __kernel_vsyscall ()
#1 0xf7bac46b in syscall () from /lib/libc.so.6
#2 0x0804995d in std::__detail::__platform_wait (
__addr=0x804d680 , __val=3)
at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:104
#3 0x08049e3a in std::__detail::__waiter_pool::_M_do_wait (__old=3,
__addr=0x804d680 , this=)
at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:261
#4 std::__detail::__waiter
>::_M_do_wait_v(unsigned
int const*, std::remove_volatile::type,
std::memory_order)::{lambda()#1}>(unsigned int,
std::__atomic_impl::wait(unsigned int const*,
std::remove_volatile::type, std::memory_order)::{lambda()#1})
(__vfn=...,
__old=42, this=)
at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:400
#5 std::__atomic_wait_address_v(unsigned int const*,
std::remove_volatile::type,
std::memory_order)::{lambda()#1}>(unsigned int const*, unsigned int,
std::__atomic_impl::wa--Type for more, q to quit, c to continue without
paging--
it(unsigned int const*, std::remove_volatile::type,
std::memory_order)::{lambda()#1}) (__addr=0xffeb402c, __old=42, __vfn=...)
at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:430
#6 0x08049ef2 in std::__atomic_impl::wait (
__m=std::memory_order::seq_cst, __old=,
__ptr=)
at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_base.h:1018
#7 std::__atomic_ref::wait (
__m=std::memory_order::seq_cst, __old=, this=0xffeb4034)
at
/export/gnu/import/git/gcc-test-master-intel64-native/bld/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_base.h:1570
#8 test (va=0, vb=42)
at
/export/gnu/import/git/gcc-test-master-intel64-native/src-master/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:44
#9 0x0804926b in main ()
at
/export/gnu/import/git/gcc-test-master-intel64-native/src-master/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:54
(gdb)
GCC was configured with
--with-arch=native --with-cpu=native --prefix=/usr/12.0.0 --enable-clocale=gnu
--with-system-zlib --enable-shared --enable-cet --with-demangler-in-ld
--enable-libmpx --with-multilib-list=m32,m64,mx32 --with-fpmath=sse
It happens about once a few weeks.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 Uroš Bizjak changed: What|Removed |Added Resolution|--- |FIXED Status|REOPENED|RESOLVED --- Comment #36 from Uroš Bizjak --- Fixed by reverting PR71245.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #35 from CVS Commits --- The releases/gcc-9 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:5972907bd0528a04eb8fa4f9897714724da53f04 commit r9-9634-g5972907bd0528a04eb8fa4f9897714724da53f04 Author: Uros Bizjak Date: Tue Jul 20 06:38:46 2021 +0200 i386: Remove atomic_storedi_fpu and atomic_loaddi_fpu peepholes [PR100182] These patterns result in non-atomic sequence. 2021-07-21 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (define_peephole2 atomic_storedi_fpu): Remove. (define_peephole2 atomic_loaddi_fpu): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr71245-1.c: Remove. * gcc.target/i386/pr71245-2.c: Ditto.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #34 from CVS Commits --- The releases/gcc-10 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:ff3a8cd277752fd6167c39c00391662e47f1d1c0 commit r10-9992-gff3a8cd277752fd6167c39c00391662e47f1d1c0 Author: Uros Bizjak Date: Tue Jul 20 06:36:02 2021 +0200 i386: Remove atomic_storedi_fpu and atomic_loaddi_fpu peepholes [PR100182] These patterns result in non-atomic sequence. 2021-07-21 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (define_peephole2 atomic_storedi_fpu): Remove. (define_peephole2 atomic_loaddi_fpu): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr71245-1.c: Remove. * gcc.target/i386/pr71245-2.c: Ditto.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #33 from CVS Commits --- The releases/gcc-11 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:f2060ae92f22e4877af21018cf3b8cc2eca4745e commit r11-8783-gf2060ae92f22e4877af21018cf3b8cc2eca4745e Author: Uros Bizjak Date: Tue Jul 20 06:29:39 2021 +0200 i386: Remove atomic_storedi_fpu and atomic_loaddi_fpu peepholes [PR100182] These patterns result in non-atomic sequence. 2021-07-21 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (define_peephole2 atomic_storedi_fpu): Remove. (define_peephole2 atomic_loaddi_fpu): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr71245-1.c: Remove. * gcc.target/i386/pr71245-2.c: Ditto.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #32 from CVS Commits --- The master branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:6d4da4aeef5b20f7f9693ddc27d26740d0dbe36c commit r12-2407-g6d4da4aeef5b20f7f9693ddc27d26740d0dbe36c Author: Uros Bizjak Date: Tue Jul 20 06:15:16 2021 +0200 i386: Remove atomic_storedi_fpu and atomic_loaddi_fpu peepholes [PR100182] These patterns result in non-atomic sequence. 2021-07-21 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (define_peephole2 atomic_storedi_fpu): Remove. (define_peephole2 atomic_loaddi_fpu): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr71245-1.c: Remove. * gcc.target/i386/pr71245-2.c: Ditto.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #31 from Uroš Bizjak --- (In reply to H.J. Lu from comment #30) > (In reply to Uroš Bizjak from comment #29) > > (In reply to H.J. Lu from comment #28) > > > 29_atomics/atomic_ref/wait_notify.c has the same issue on Linux/x86-64 > > > with > > > -m32: > > > > Are you sure? The mentioned peephole2 patterns now emit only x87 or SSE > > DFmode loads/stores that are guaranteed to be atomic. > > It does happen, but not very often. OK, I will simply remove *all* these peephole2s.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #30 from H.J. Lu --- (In reply to Uroš Bizjak from comment #29) > (In reply to H.J. Lu from comment #28) > > 29_atomics/atomic_ref/wait_notify.c has the same issue on Linux/x86-64 with > > -m32: > > Are you sure? The mentioned peephole2 patterns now emit only x87 or SSE > DFmode loads/stores that are guaranteed to be atomic. It does happen, but not very often.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #29 from Uroš Bizjak --- (In reply to H.J. Lu from comment #28) > 29_atomics/atomic_ref/wait_notify.c has the same issue on Linux/x86-64 with > -m32: Are you sure? The mentioned peephole2 patterns now emit only x87 or SSE DFmode loads/stores that are guaranteed to be atomic.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182
H.J. Lu changed:
What|Removed |Added
Status|RESOLVED|REOPENED
Resolution|FIXED |---
--- Comment #28 from H.J. Lu ---
29_atomics/atomic_ref/wait_notify.c has the same issue on Linux/x86-64 with
-m32:
(gdb) bt
#0 0xf7f5455d in __kernel_vsyscall ()
#1 0xf7b3a46b in syscall () from /lib/libc.so.6
#2 0x0804995d in std::__detail::__platform_wait (
__addr=0x804d480 , __val=1)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:104
#3 0x08049afa in std::__detail::__waiter_pool::_M_do_wait (__old=1,
__addr=0x804d480 , this=)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:261
#4 std::__detail::__waiter
>::_M_do_wait_v(int const*,
std::remove_volatile::type, std::memory_order)::{lambda()#1}>(int,
std::__atomic_impl::wait(int const*, std::remove_volatile::type,
std::memory_order)::{lambda()#1}) (__vfn=...,
__old=42, this=)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:400
#5 std::__atomic_wait_address_v(int const*,
std::remove_volatile::type, std::memory_order)::{lambda()#1}>(int const*,
int, std::__atomic_impl::wait(int const*, std::remove_volatile::type,
std::memory_order)::{lambda()#1}) (__addr=0xffaf629c, __old=42, __vfn=...)
--Type for more, q to quit, c to continue without paging--
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_wait.h:430
#6 0x08049bb2 in std::__atomic_impl::wait (
__m=std::memory_order::seq_cst, __old=,
__ptr=)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_base.h:1018
#7 std::__atomic_ref::wait (__m=std::memory_order::seq_cst,
__old=, this=0xffaf62a4)
at
/export/users/hjl/build/gnu/tools-build/gcc-gitlab-native/build-x86_64-linux/x86_64-pc-linux-gnu/32/libstdc++-v3/include/bits/atomic_base.h:1570
#8 test (va=0, vb=42)
at
/export/gnu/import/git/gitlab/x86-gcc/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:44
#9 0x08049250 in main ()
at
/export/gnu/import/git/gitlab/x86-gcc/libstdc++-v3/testsuite/29_atomics/atomic_ref/wait_notify.cc:52
(gdb)
when GCC is configured with
--enable-cet --with-demangler-in-ld --prefix=/usr/gcc-12.0.0-native
--with-local-prefix=/usr/local --enable-gnu-indirect-function
--enable-clocale=gnu --with-system-zlib --with-target-system-zlib
--with-fpmath=sse --with-arch=native --with-cpu=native
--enable-languages=c,c++,fortran,lto,objc,ada,obj-c++,go
where native == skylake-avx512. It happens one out of ~10 runs.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 Uroš Bizjak changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #27 from Uroš Bizjak --- Fixed everywhere.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #26 from CVS Commits --- The releases/gcc-8 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:0d277114b4b2d0cb386c7abe409a81ca29d9d61d commit r8-10926-g0d277114b4b2d0cb386c7abe409a81ca29d9d61d Author: Uros Bizjak Date: Fri Apr 23 17:29:29 2021 +0200 i386: Fix atomic FP peepholes [PR100182] 64bit loads to/stores from x87 and SSE registers are atomic also on 32-bit targets, so there is no need for additional atomic moves to a temporary register. Introduced load peephole2 patterns assume that there won't be any additional loads from the load location outside the peepholed sequence and wrongly removed the source location initialization. OTOH, introduced store peephole2 patterns assume there won't be any additional loads from the stored location outside the peepholed sequence and wrongly removed the destination location initialization. Note that we can't use plain x87 FST instruction to initialize destination location because FST converts the value to the double-precision format, changing bits during move. The patch restores removed initializations in load and store patterns. Additionally, plain x87 FST in store peephole2 patterns is prevented by limiting the store operand source to SSE registers. 2021-04-23 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (FILD_ATOMIC/FIST_ATOMIC FP load peephole2): Copy operand 3 to operand 4. Use sse_reg_operand as operand 3 predicate. (FILD_ATOMIC/FIST_ATOMIC FP load peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP load peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP load peephole2 with mem blockage): Ditto. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2): Copy operand 1 to operand 0. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP store peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP store peephole2 with mem blockage): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr100182.c: New test. * gcc.target/i386/pr71245-1.c (dg-final): Xfail scan-assembler-not. * gcc.target/i386/pr71245-2.c (dg-final): Ditto. (cherry picked from commit d2324a5ab3ff097864ae6828cb1db4dd013c70d1)
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #25 from CVS Commits --- The releases/gcc-9 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:be20ca1d4ff79baf7425a48bb887495e1ea8f788 commit r9-9472-gbe20ca1d4ff79baf7425a48bb887495e1ea8f788 Author: Uros Bizjak Date: Fri Apr 23 17:29:29 2021 +0200 i386: Fix atomic FP peepholes [PR100182] 64bit loads to/stores from x87 and SSE registers are atomic also on 32-bit targets, so there is no need for additional atomic moves to a temporary register. Introduced load peephole2 patterns assume that there won't be any additional loads from the load location outside the peepholed sequence and wrongly removed the source location initialization. OTOH, introduced store peephole2 patterns assume there won't be any additional loads from the stored location outside the peepholed sequence and wrongly removed the destination location initialization. Note that we can't use plain x87 FST instruction to initialize destination location because FST converts the value to the double-precision format, changing bits during move. The patch restores removed initializations in load and store patterns. Additionally, plain x87 FST in store peephole2 patterns is prevented by limiting the store operand source to SSE registers. 2021-04-23 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (FILD_ATOMIC/FIST_ATOMIC FP load peephole2): Copy operand 3 to operand 4. Use sse_reg_operand as operand 3 predicate. (FILD_ATOMIC/FIST_ATOMIC FP load peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP load peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP load peephole2 with mem blockage): Ditto. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2): Copy operand 1 to operand 0. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP store peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP store peephole2 with mem blockage): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr100182.c: New test. * gcc.target/i386/pr71245-1.c (dg-final): Xfail scan-assembler-not. * gcc.target/i386/pr71245-2.c (dg-final): Ditto. (cherry picked from commit d2324a5ab3ff097864ae6828cb1db4dd013c70d1)
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #24 from CVS Commits --- The releases/gcc-10 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:39e8bfe7217898e8d21bcc55efe6992fbde262f1 commit r10-9775-g39e8bfe7217898e8d21bcc55efe6992fbde262f1 Author: Uros Bizjak Date: Fri Apr 23 17:29:29 2021 +0200 i386: Fix atomic FP peepholes [PR100182] 64bit loads to/stores from x87 and SSE registers are atomic also on 32-bit targets, so there is no need for additional atomic moves to a temporary register. Introduced load peephole2 patterns assume that there won't be any additional loads from the load location outside the peepholed sequence and wrongly removed the source location initialization. OTOH, introduced store peephole2 patterns assume there won't be any additional loads from the stored location outside the peepholed sequence and wrongly removed the destination location initialization. Note that we can't use plain x87 FST instruction to initialize destination location because FST converts the value to the double-precision format, changing bits during move. The patch restores removed initializations in load and store patterns. Additionally, plain x87 FST in store peephole2 patterns is prevented by limiting the store operand source to SSE registers. 2021-04-23 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (FILD_ATOMIC/FIST_ATOMIC FP load peephole2): Copy operand 3 to operand 4. Use sse_reg_operand as operand 3 predicate. (FILD_ATOMIC/FIST_ATOMIC FP load peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP load peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP load peephole2 with mem blockage): Ditto. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2): Copy operand 1 to operand 0. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP store peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP store peephole2 with mem blockage): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr100182.c: New test. * gcc.target/i386/pr71245-1.c (dg-final): Xfail scan-assembler-not. * gcc.target/i386/pr71245-2.c (dg-final): Ditto. (cherry picked from commit d2324a5ab3ff097864ae6828cb1db4dd013c70d1)
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #23 from CVS Commits --- The releases/gcc-11 branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:c03f3077b1517a01c917f75179100f9d10b39156 commit r11-8313-gc03f3077b1517a01c917f75179100f9d10b39156 Author: Uros Bizjak Date: Wed Apr 28 12:30:04 2021 +0200 i386: Fix atomic FP peepholes [PR100182] 64bit loads to/stores from x87 and SSE registers are atomic also on 32-bit targets, so there is no need for additional atomic moves to a temporary register. Introduced load peephole2 patterns assume that there won't be any additional loads from the load location outside the peepholed sequence and wrongly removed the source location initialization. OTOH, introduced store peephole2 patterns assume there won't be any additional loads from the stored location outside the peepholed sequence and wrongly removed the destination location initialization. Note that we can't use plain x87 FST instruction to initialize destination location because FST converts the value to the double-precision format, changing bits during move. The patch restores removed initializations in load and store patterns. Additionally, plain x87 FST in store peephole2 patterns is prevented by limiting the store operand source to SSE registers. 2021-04-27 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (FILD_ATOMIC/FIST_ATOMIC FP load peephole2): Copy operand 3 to operand 4. Use sse_reg_operand as operand 3 predicate. (FILD_ATOMIC/FIST_ATOMIC FP load peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP load peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP load peephole2 with mem blockage): Ditto. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2): Copy operand 1 to operand 0. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP store peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP store peephole2 with mem blockage): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr100182.c: New test. * gcc.target/i386/pr71245-1.c (dg-final): Xfail scan-assembler-not. * gcc.target/i386/pr71245-2.c (dg-final): Ditto.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #22 from CVS Commits --- The master branch has been updated by Uros Bizjak : https://gcc.gnu.org/g:d2324a5ab3ff097864ae6828cb1db4dd013c70d1 commit r12-91-gd2324a5ab3ff097864ae6828cb1db4dd013c70d1 Author: Uros Bizjak Date: Fri Apr 23 17:29:29 2021 +0200 i386: Fix atomic FP peepholes [PR100182] 64bit loads to/stores from x87 and SSE registers are atomic also on 32-bit targets, so there is no need for additional atomic moves to a temporary register. Introduced load peephole2 patterns assume that there won't be any additional loads from the load location outside the peepholed sequence and wrongly removed the source location initialization. OTOH, introduced store peephole2 patterns assume there won't be any additional loads from the stored location outside the peepholed sequence and wrongly removed the destination location initialization. Note that we can't use plain x87 FST instruction to initialize destination location because FST converts the value to the double-precision format, changing bits during move. The patch restores removed initializations in load and store patterns. Additionally, plain x87 FST in store peephole2 patterns is prevented by limiting the store operand source to SSE registers. 2021-04-23 Uroš Bizjak gcc/ PR target/100182 * config/i386/sync.md (FILD_ATOMIC/FIST_ATOMIC FP load peephole2): Copy operand 3 to operand 4. Use sse_reg_operand as operand 3 predicate. (FILD_ATOMIC/FIST_ATOMIC FP load peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP load peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP load peephole2 with mem blockage): Ditto. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2): Copy operand 1 to operand 0. (FILD_ATOMIC/FIST_ATOMIC FP store peephole2 with mem blockage): Ditto. (LDX_ATOMIC/STX_ATOMIC FP store peephole2): Ditto. (LDX_ATOMIC/LDX_ATOMIC FP store peephole2 with mem blockage): Ditto. gcc/testsuite/ PR target/100182 * gcc.target/i386/pr100182.c: New test. * gcc.target/i386/pr71245-1.c (dg-final): Xfail scan-assembler-not. * gcc.target/i386/pr71245-2.c (dg-final): Ditto.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 Uroš Bizjak changed: What|Removed |Added Attachment #50649|0 |1 is obsolete|| --- Comment #21 from Uroš Bizjak --- Created attachment 50659 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50659=edit Proposed patch Here is the complete proposed patch. We can retain problematic atomic store followed by a DFmode load peepholes as long as we have a load to the SSE register. Load to the SSE register uses movlps/movq moves that preserve all bits, so we are sure the store to a memory location is unchanged from the original. However, "load to the SSE register" requirement makes the peephole ineffective for -mfpmath=387, so XFAILs are added to affected testcases.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 Jakub Jelinek changed: What|Removed |Added CC||aoliva at gcc dot gnu.org --- Comment #20 from Jakub Jelinek --- *** Bug 100228 has been marked as a duplicate of this bug. ***
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #19 from Jakub Jelinek --- Perhaps best would be to try to construct a testcase for each of the peephole2s and try some bit pattern that isn't preserved through the FPU except for fistp/fildp and see what enabling/disabling each of the peephole2s does to it.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #18 from Jakub Jelinek --- Indeed.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #17 from Uroš Bizjak --- (In reply to Uroš Bizjak from comment #16) > (In reply to Jakub Jelinek from comment #15) > > Yes, but do they preserve all the bits and never modify any bit patterns, > > including qNaNs and sNaNs? I thought the point of using the fistp was that > > it preserves everything. > > Hm, they don't... This probably means we have to remove x87 peepholes, where an atomic store is followed by a DFmode read. x87 can't load and store DFmode untouched without fild/fistp pair.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #16 from Uroš Bizjak --- (In reply to Jakub Jelinek from comment #15) > Yes, but do they preserve all the bits and never modify any bit patterns, > including qNaNs and sNaNs? I thought the point of using the fistp was that > it preserves everything. Hm, they don't...
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #15 from Jakub Jelinek --- Yes, but do they preserve all the bits and never modify any bit patterns, including qNaNs and sNaNs? I thought the point of using the fistp was that it preserves everything.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #14 from Uroš Bizjak --- (In reply to Uroš Bizjak from comment #13) > DFmode loads and stores *are* atomic, this is what the optimization is based > on. Loads and stores to/from x87 and SSE registers, to be clear.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #13 from Uroš Bizjak --- (In reply to Jakub Jelinek from comment #12) > They do. Though, in the combined patch I'm still a little bit worried about > the first 4 modified peephole2s, the last 4 look good to me. > The last 4 are where the original insn did a normal DFmode store and your > patch restores those DFmode stores. > But the first 4 had an atomic store followed by a DFmode read, shouldn't > those > preserve an atomic store instead of the DFmode store? A non-atomic DFmode > read is one thing, but it could be followed later by atomic loads, both into > DFmode and ones into DImode that would check the whole bit pattern. DFmode loads and stores *are* atomic, this is what the optimization is based on.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #12 from Jakub Jelinek --- They do. Though, in the combined patch I'm still a little bit worried about the first 4 modified peephole2s, the last 4 look good to me. The last 4 are where the original insn did a normal DFmode store and your patch restores those DFmode stores. But the first 4 had an atomic store followed by a DFmode read, shouldn't those preserve an atomic store instead of the DFmode store? A non-atomic DFmode read is one thing, but it could be followed later by atomic loads, both into DFmode and ones into DImode that would check the whole bit pattern.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #11 from Uroš Bizjak --- Jakub, do these two patches fix your failures?
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182
--- Comment #10 from Uroš Bizjak ---
(In reply to Uroš Bizjak from comment #9)
> (In reply to Jakub Jelinek from comment #8)
> > I think there are 8 those peephole2s rather than just 4 (I've been looking
> > for
> > rtx_equal_p (XEXP.*, 0) in sync.md
>
> No, the other are not problematic.
Actually, you are right. Those other peephole2 sequences also write to the
memory and it is assumed, that the memory is not accessed outside the sequence.
Additional patch follows:
--cut here--
diff --git a/gcc/config/i386/sync.md b/gcc/config/i386/sync.md
index c7c508c8de8..c95cf50970e 100644
--- a/gcc/config/i386/sync.md
+++ b/gcc/config/i386/sync.md
@@ -231,7 +231,8 @@
"!TARGET_64BIT
&& peep2_reg_dead_p (2, operands[0])
&& rtx_equal_p (XEXP (operands[4], 0), XEXP (operands[2], 0))"
- [(set (match_dup 3) (match_dup 5))]
+ [(set (match_dup 3) (match_dup 5))
+ (set (match_dup 4) (match_dup 3))]
"operands[5] = gen_lowpart (DFmode, operands[1]);")
(define_peephole2
@@ -251,6 +252,7 @@
[(const_int 0)]
{
emit_move_insn (operands[3], gen_lowpart (DFmode, operands[1]));
+ emit_move_insn (operands[4], operands[3]);
emit_insn (gen_memory_blockage ());
DONE;
})
@@ -267,7 +269,8 @@
"!TARGET_64BIT
&& peep2_reg_dead_p (2, operands[0])
&& rtx_equal_p (XEXP (operands[4], 0), XEXP (operands[2], 0))"
- [(set (match_dup 3) (match_dup 5))]
+ [(set (match_dup 3) (match_dup 5))
+ (set (match_dup 4) (match_dup 3))]
"operands[5] = gen_lowpart (DFmode, operands[1]);")
(define_peephole2
@@ -287,6 +290,7 @@
[(const_int 0)]
{
emit_move_insn (operands[3], gen_lowpart (DFmode, operands[1]));
+ emit_move_insn (operands[4], operands[3]);
emit_insn (gen_memory_blockage ());
DONE;
})
--cut here--
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #9 from Uroš Bizjak --- (In reply to Jakub Jelinek from comment #8) > I think there are 8 those peephole2s rather than just 4 (I've been looking > for > rtx_equal_p (XEXP.*, 0) in sync.md No, the other are not problematic.
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #8 from Jakub Jelinek --- I think there are 8 those peephole2s rather than just 4 (I've been looking for rtx_equal_p (XEXP.*, 0) in sync.md
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182
Uroš Bizjak changed:
What|Removed |Added
Status|NEW |ASSIGNED
CC|uros at gcc dot gnu.org|
Assignee|unassigned at gcc dot gnu.org |ubizjak at gmail dot com
--- Comment #7 from Uroš Bizjak ---
(In reply to Jakub Jelinek from comment #1)
> In this particular case it is the sync.md:398 peephole2:
> (define_peephole2
> [(set (match_operand:DF 0 "memory_operand")
> (match_operand:DF 1 "any_fp_register_operand"))
>(set (mem:BLK (scratch:SI))
> (unspec:BLK [(mem:BLK (scratch:SI))] UNSPEC_MEMORY_BLOCKAGE))
>(set (match_operand:DF 2 "fp_register_operand")
> (unspec:DF [(match_operand:DI 3 "memory_operand")]
>UNSPEC_FILD_ATOMIC))
>(set (match_operand:DI 4 "memory_operand")
> (unspec:DI [(match_dup 2)]
>UNSPEC_FIST_ATOMIC))]
> "!TARGET_64BIT
>&& peep2_reg_dead_p (4, operands[2])
>&& rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
> [(const_int 0)]
> {
> emit_insn (gen_memory_blockage ());
> emit_move_insn (gen_lowpart (DFmode, operands[4]), operands[1]);
> DONE;
> })
> that triggers here but from what I can read, all the r7-1112 peephole2s
> optimize away stores to some memory on the assumption that the memory is
> read only once (in another insn matched by the same peephole2).
> I'm not 100% sure if we can rely for it on spill slots for which r7-112
> seems to have been written, but for other memory we'd need to prove that the
> memory is dead.
> Rather than removing those peephole2s altogether, I wonder if we just
> shouldn't check that the memory_operand which we'd optimize away stores to
> aren't spill slots.
Actually, these peepholes are too eager and also remove the store to the memory
operand 0 on the assumption that the operand is used only in the peephole2
sequence. As shown in the testcase, this is not always true, and operand 0 can
be accessed also after the peephole2'd sequence.
The solution is to not remove the store to operand 0. Probably there will be
some unneeded stores left in the code, but IMO, this is a small price to pay
for the correctness. And we still remove fild/fistp pair.
I'm testing the following patch:
--cut here--
diff --git a/gcc/config/i386/sync.md b/gcc/config/i386/sync.md
index c7c508c8de8..538d1f89497 100644
--- a/gcc/config/i386/sync.md
+++ b/gcc/config/i386/sync.md
@@ -392,7 +392,8 @@
"!TARGET_64BIT
&& peep2_reg_dead_p (3, operands[2])
&& rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
- [(set (match_dup 5) (match_dup 1))]
+ [(set (match_dup 0) (match_dup 1))
+ (set (match_dup 5) (match_dup 1))]
"operands[5] = gen_lowpart (DFmode, operands[4]);")
(define_peephole2
@@ -411,6 +412,7 @@
&& rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
[(const_int 0)]
{
+ emit_move_insn (operands[0], operands[1]);
emit_insn (gen_memory_blockage ());
emit_move_insn (gen_lowpart (DFmode, operands[4]), operands[1]);
DONE;
@@ -428,7 +430,8 @@
"!TARGET_64BIT
&& peep2_reg_dead_p (3, operands[2])
&& rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
- [(set (match_dup 5) (match_dup 1))]
+ [(set (match_dup 0) (match_dup 1))
+ (set (match_dup 5) (match_dup 1))]
"operands[5] = gen_lowpart (DFmode, operands[4]);")
(define_peephole2
@@ -447,6 +450,7 @@
&& rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
[(const_int 0)]
{
+ emit_move_insn (operands[0], operands[1]);
emit_insn (gen_memory_blockage ());
emit_move_insn (gen_lowpart (DFmode, operands[4]), operands[1]);
DONE;
--cut here--
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #6 from CVS Commits --- The releases/gcc-11 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:a21f3b38c3b9a5c28c79be37b040e7d06d827d76 commit r11-8281-ga21f3b38c3b9a5c28c79be37b040e7d06d827d76 Author: Jakub Jelinek Date: Thu Apr 22 15:08:21 2021 +0200 libstdc++: Add workaround for ia32 floating atomics miscompilations [PR100184] gcc on ia32 miscompiles various atomics involving floating point, unfortunately I'm afraid it is too late to fix that for 11.1 and as I'm quite lost on it, it might take a while for 12 too (disabling all the 8 peephole2s would be easiest, but then we'd run into optimization regressions). While 1.cc just FAILs, with dejagnu 1.6.1 wait_notify.cc hangs the make check even after the timeout fires. The following patch therefore xfails the former and skips the latter. Tested on x86_64-linux where make check RUNTESTFLAGS='conformance.exp=atomic_float/*.cc' is still === libstdc++ Summary === # of expected passes8 and on i686-linux, where it is now === libstdc++ Summary === # of expected passes5 # of expected failures 1 # of unsupported tests 1 2021-04-22 Jakub Jelinek PR target/100182 * testsuite/29_atomics/atomic_float/1.cc: Add dg-xfail-run-if for ia32. * testsuite/29_atomics/atomic_float/wait_notify.cc: Add dg-skip-if for ia32. (cherry picked from commit 0f4588141fcbe4e0f1fa12776b47200870f6c621)
[Bug target/100182] [8/9/10/11/12 Regression] Miscompilation of atomic_float/1.cc and atomic_float/wait_notify.cc on i686
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100182 --- Comment #5 from CVS Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:0f4588141fcbe4e0f1fa12776b47200870f6c621 commit r12-60-g0f4588141fcbe4e0f1fa12776b47200870f6c621 Author: Jakub Jelinek Date: Thu Apr 22 15:08:21 2021 +0200 libstdc++: Add workaround for ia32 floating atomics miscompilations [PR100184] gcc on ia32 miscompiles various atomics involving floating point, unfortunately I'm afraid it is too late to fix that for 11.1 and as I'm quite lost on it, it might take a while for 12 too (disabling all the 8 peephole2s would be easiest, but then we'd run into optimization regressions). While 1.cc just FAILs, with dejagnu 1.6.1 wait_notify.cc hangs the make check even after the timeout fires. The following patch therefore xfails the former and skips the latter. Tested on x86_64-linux where make check RUNTESTFLAGS='conformance.exp=atomic_float/*.cc' is still === libstdc++ Summary === # of expected passes8 and on i686-linux, where it is now === libstdc++ Summary === # of expected passes5 # of expected failures 1 # of unsupported tests 1 2021-04-22 Jakub Jelinek PR target/100182 * testsuite/29_atomics/atomic_float/1.cc: Add dg-xfail-run-if for ia32. * testsuite/29_atomics/atomic_float/wait_notify.cc: Add dg-skip-if for ia32.
