Message ID | 20230913123226.2083892-1-jwakely@redhat.com |
---|---|
State | New |
Headers | show |
Series | libstdc++: Remove some more unconditional uses of atomics | expand |
Hi, On Wed, 13 Sept 2023 at 14:32, Jonathan Wakely <jwakely@redhat.com> wrote: > Tested x86_64-linux and aarch64-linux. I intend to push this to trunk. > > -- >8 -- > > These atomics cause linker errors on arm4t where __sync_synchronize is > not defined. For single-threaded targets we don't need the atomics. > > I ran the tests on arm-eabi default config (so, armv4t) with this patch, and here is the list of remaining UNRESOLVED tests: 29_atomics/atomic/compare_exchange_padding.cc 29_atomics/atomic/cons/value_init.cc 29_atomics/atomic_float/value_init.cc 29_atomics/atomic_integral/cons/value_init.cc 29_atomics/atomic_ref/compare_exchange_padding.cc 29_atomics/atomic_ref/generic.cc 29_atomics/atomic_ref/integral.cc 29_atomics/atomic_ref/pointer.cc experimental/polymorphic_allocator/construct_pair.cc all of them are due to undefined reference to __sync_synchronize (some also reference __atomic_compare_exchange_4, etc...) IIUC, this should not be the case for experimental/polymorphic_allocator/construct_pair.cc ? The reference for __sync_synchronize is near the beginning of test0[123] from a call to __atomic_load_n line 835 of atomic_base.h not sure where it comes from, the .loc directive indicates line 28 of the testcase which is the opening brace HTH Christophe libstdc++-v3/ChangeLog: > > * include/experimental/io_context (io_context) > [!_GLIBCXX_HAS_GTHREADS]: > Use a plain integer for _M_work_count for single-threaded > targets. > * src/c++17/memory_resource.cc [!_GLIBCXX_HAS_GTHREADS] > (atomic_mem_res): Use unsynchronized type for single-threaded > targets. > --- > libstdc++-v3/include/experimental/io_context | 4 ++ > libstdc++-v3/src/c++17/memory_resource.cc | 49 ++++++++++---------- > 2 files changed, 29 insertions(+), 24 deletions(-) > > diff --git a/libstdc++-v3/include/experimental/io_context > b/libstdc++-v3/include/experimental/io_context > index c59f8c8e73b..c878d5a7025 100644 > --- a/libstdc++-v3/include/experimental/io_context > +++ b/libstdc++-v3/include/experimental/io_context > @@ -562,7 +562,11 @@ inline namespace v1 > } > }; > > +#ifdef _GLIBCXX_HAS_GTHREADS > atomic<count_type> _M_work_count; > +#else > + count_type _M_work_count; > +#endif > mutable execution_context::mutex_type _M_mtx; > queue<function<void()>> _M_op; > bool _M_stopped = false; > diff --git a/libstdc++-v3/src/c++17/memory_resource.cc > b/libstdc++-v3/src/c++17/memory_resource.cc > index c0c7cf0cf83..63856eadaf5 100644 > --- a/libstdc++-v3/src/c++17/memory_resource.cc > +++ b/libstdc++-v3/src/c++17/memory_resource.cc > @@ -27,9 +27,9 @@ > #include <atomic> > #include <bit> // has_single_bit, bit_ceil, > bit_width > #include <new> > +#include <bits/move.h> // std::__exchange > #if ATOMIC_POINTER_LOCK_FREE != 2 > # include <bits/std_mutex.h> // std::mutex, std::lock_guard > -# include <bits/move.h> // std::__exchange > #endif > > #if __has_cpp_attribute(clang::require_constant_initialization) > @@ -94,10 +94,31 @@ namespace pmr > > __constinit constant_init<newdel_res_t> newdel_res{}; > __constinit constant_init<null_res_t> null_res{}; > -#if ATOMIC_POINTER_LOCK_FREE == 2 > + > +#ifndef _GLIBCXX_HAS_GTHREADS > +# define _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED > + // Single-threaded, no need for synchronization > + struct atomic_mem_res > + { > + constexpr > + atomic_mem_res(memory_resource* r) : val(r) { } > + > + memory_resource* val; > + > + memory_resource* load(std::memory_order) const > + { > + return val; > + } > + > + memory_resource* exchange(memory_resource* r, std::memory_order) > + { > + return std::__exchange(val, r); > + } > + }; > +#elif ATOMIC_POINTER_LOCK_FREE == 2 > using atomic_mem_res = atomic<memory_resource*>; > # define _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED > -#elif defined(_GLIBCXX_HAS_GTHREADS) > +#else > // Can't use pointer-width atomics, define a type using a mutex > instead: > struct atomic_mem_res > { > @@ -123,27 +144,7 @@ namespace pmr > return std::__exchange(val, r); > } > }; > -#else > -# define _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED > - // Single-threaded, no need for synchronization > - struct atomic_mem_res > - { > - constexpr > - atomic_mem_res(memory_resource* r) : val(r) { } > - > - memory_resource* val; > - > - memory_resource* load(std::memory_order) const > - { > - return val; > - } > - > - memory_resource* exchange(memory_resource* r, std::memory_order) > - { > - return std::__exchange(val, r); > - } > - }; > -#endif // ATOMIC_POINTER_LOCK_FREE == 2 > +#endif > > #ifdef _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED > __constinit constant_init<atomic_mem_res> > default_res{&newdel_res.obj}; > -- > 2.41.0 > >
On Thu, 14 Sept 2023 at 08:44, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > Hi, > > > On Wed, 13 Sept 2023 at 14:32, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> Tested x86_64-linux and aarch64-linux. I intend to push this to trunk. >> >> -- >8 -- >> >> These atomics cause linker errors on arm4t where __sync_synchronize is >> not defined. For single-threaded targets we don't need the atomics. >> > > I ran the tests on arm-eabi default config (so, armv4t) with this patch, and here is the list of remaining UNRESOLVED tests: > 29_atomics/atomic/compare_exchange_padding.cc > 29_atomics/atomic/cons/value_init.cc > 29_atomics/atomic_float/value_init.cc > 29_atomics/atomic_integral/cons/value_init.cc > 29_atomics/atomic_ref/compare_exchange_padding.cc > 29_atomics/atomic_ref/generic.cc > 29_atomics/atomic_ref/integral.cc > 29_atomics/atomic_ref/pointer.cc > experimental/polymorphic_allocator/construct_pair.cc > > all of them are due to undefined reference to __sync_synchronize > (some also reference __atomic_compare_exchange_4, etc...) > > > IIUC, this should not be the case for experimental/polymorphic_allocator/construct_pair.cc ? > The reference for __sync_synchronize is near the beginning of test0[123] > from a call to __atomic_load_n line 835 of atomic_base.h > not sure where it comes from, the .loc directive indicates line 28 of the testcase which is the opening brace Doh, I removed the atomics from <memory_resource> but this is <experimental/memory_resource>, which has a separate implementation. I'll make a change to <experimental/memory_resource> as well, thanks for catching my silly mistake.
On Thu, 14 Sept 2023 at 10:17, Jonathan Wakely <jwakely@redhat.com> wrote: > On Thu, 14 Sept 2023 at 08:44, Christophe Lyon > <christophe.lyon@linaro.org> wrote: > > > > Hi, > > > > > > On Wed, 13 Sept 2023 at 14:32, Jonathan Wakely <jwakely@redhat.com> > wrote: > >> > >> Tested x86_64-linux and aarch64-linux. I intend to push this to trunk. > >> > >> -- >8 -- > >> > >> These atomics cause linker errors on arm4t where __sync_synchronize is > >> not defined. For single-threaded targets we don't need the atomics. > >> > > > > I ran the tests on arm-eabi default config (so, armv4t) with this patch, > and here is the list of remaining UNRESOLVED tests: > > 29_atomics/atomic/compare_exchange_padding.cc > > 29_atomics/atomic/cons/value_init.cc > > 29_atomics/atomic_float/value_init.cc > > 29_atomics/atomic_integral/cons/value_init.cc > > 29_atomics/atomic_ref/compare_exchange_padding.cc > > 29_atomics/atomic_ref/generic.cc > > 29_atomics/atomic_ref/integral.cc > > 29_atomics/atomic_ref/pointer.cc > > experimental/polymorphic_allocator/construct_pair.cc > > > > all of them are due to undefined reference to __sync_synchronize > > (some also reference __atomic_compare_exchange_4, etc...) > > > > > > IIUC, this should not be the case for > experimental/polymorphic_allocator/construct_pair.cc ? > > The reference for __sync_synchronize is near the beginning of test0[123] > > from a call to __atomic_load_n line 835 of atomic_base.h > > not sure where it comes from, the .loc directive indicates line 28 of > the testcase which is the opening brace > > Doh, I removed the atomics from <memory_resource> but this is > <experimental/memory_resource>, which has a separate implementation. > > I'll make a change to <experimental/memory_resource> as well, thanks > for catching my silly mistake. > > You're welcome. So I'll shrink my patch and add dg-require-thread-fence only to the few 29_atomics tests listed above. Christophe
On Thu, 14 Sept 2023 at 09:41, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > > On Thu, 14 Sept 2023 at 10:17, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> On Thu, 14 Sept 2023 at 08:44, Christophe Lyon >> <christophe.lyon@linaro.org> wrote: >> > >> > Hi, >> > >> > >> > On Wed, 13 Sept 2023 at 14:32, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> >> >> Tested x86_64-linux and aarch64-linux. I intend to push this to trunk. >> >> >> >> -- >8 -- >> >> >> >> These atomics cause linker errors on arm4t where __sync_synchronize is >> >> not defined. For single-threaded targets we don't need the atomics. >> >> >> > >> > I ran the tests on arm-eabi default config (so, armv4t) with this patch, and here is the list of remaining UNRESOLVED tests: >> > 29_atomics/atomic/compare_exchange_padding.cc >> > 29_atomics/atomic/cons/value_init.cc >> > 29_atomics/atomic_float/value_init.cc >> > 29_atomics/atomic_integral/cons/value_init.cc >> > 29_atomics/atomic_ref/compare_exchange_padding.cc >> > 29_atomics/atomic_ref/generic.cc >> > 29_atomics/atomic_ref/integral.cc >> > 29_atomics/atomic_ref/pointer.cc >> > experimental/polymorphic_allocator/construct_pair.cc >> > >> > all of them are due to undefined reference to __sync_synchronize >> > (some also reference __atomic_compare_exchange_4, etc...) >> > >> > >> > IIUC, this should not be the case for experimental/polymorphic_allocator/construct_pair.cc ? >> > The reference for __sync_synchronize is near the beginning of test0[123] >> > from a call to __atomic_load_n line 835 of atomic_base.h >> > not sure where it comes from, the .loc directive indicates line 28 of the testcase which is the opening brace >> >> Doh, I removed the atomics from <memory_resource> but this is >> <experimental/memory_resource>, which has a separate implementation. >> >> I'll make a change to <experimental/memory_resource> as well, thanks >> for catching my silly mistake. >> > > You're welcome. > So I'll shrink my patch and add dg-require-thread-fence only to the few 29_atomics tests listed above. Great, thanks. That's approved for trunk then. N.B. if you'd prefer to add { dg-require-effective-target thread_fence } instead of { dg-require-thread-fence "" } then that's fine, just note that the effective target uses an underscore not a hyphen. The dg-require-thread-fence proc just uses the proc that checks the thread_fence effective target, so both forms do the same thing.
On Thu, 14 Sept 2023 at 11:06, Jonathan Wakely <jwakely@redhat.com> wrote: > On Thu, 14 Sept 2023 at 09:41, Christophe Lyon > <christophe.lyon@linaro.org> wrote: > > > > > > > > On Thu, 14 Sept 2023 at 10:17, Jonathan Wakely <jwakely@redhat.com> > wrote: > >> > >> On Thu, 14 Sept 2023 at 08:44, Christophe Lyon > >> <christophe.lyon@linaro.org> wrote: > >> > > >> > Hi, > >> > > >> > > >> > On Wed, 13 Sept 2023 at 14:32, Jonathan Wakely <jwakely@redhat.com> > wrote: > >> >> > >> >> Tested x86_64-linux and aarch64-linux. I intend to push this to > trunk. > >> >> > >> >> -- >8 -- > >> >> > >> >> These atomics cause linker errors on arm4t where __sync_synchronize > is > >> >> not defined. For single-threaded targets we don't need the atomics. > >> >> > >> > > >> > I ran the tests on arm-eabi default config (so, armv4t) with this > patch, and here is the list of remaining UNRESOLVED tests: > >> > 29_atomics/atomic/compare_exchange_padding.cc > >> > 29_atomics/atomic/cons/value_init.cc > >> > 29_atomics/atomic_float/value_init.cc > >> > 29_atomics/atomic_integral/cons/value_init.cc > >> > 29_atomics/atomic_ref/compare_exchange_padding.cc > >> > 29_atomics/atomic_ref/generic.cc > >> > 29_atomics/atomic_ref/integral.cc > >> > 29_atomics/atomic_ref/pointer.cc > >> > experimental/polymorphic_allocator/construct_pair.cc > >> > > >> > all of them are due to undefined reference to __sync_synchronize > >> > (some also reference __atomic_compare_exchange_4, etc...) > >> > > >> > > >> > IIUC, this should not be the case for > experimental/polymorphic_allocator/construct_pair.cc ? > >> > The reference for __sync_synchronize is near the beginning of > test0[123] > >> > from a call to __atomic_load_n line 835 of atomic_base.h > >> > not sure where it comes from, the .loc directive indicates line 28 of > the testcase which is the opening brace > >> > >> Doh, I removed the atomics from <memory_resource> but this is > >> <experimental/memory_resource>, which has a separate implementation. > >> > >> I'll make a change to <experimental/memory_resource> as well, thanks > >> for catching my silly mistake. > >> > > > > You're welcome. > > So I'll shrink my patch and add dg-require-thread-fence only to the few > 29_atomics tests listed above. > > Great, thanks. That's approved for trunk then. > > N.B. if you'd prefer to add { dg-require-effective-target thread_fence > } instead of { dg-require-thread-fence "" } then that's fine, just > note that the effective target uses an underscore not a hyphen. The > dg-require-thread-fence proc just uses the proc that checks the > thread_fence effective target, so both forms do the same thing. > > Ha! Just sent v2, I kept dg-require-thread-fence, because it was used elsewhere in the libstsdc++ testsuite. Thanks, Christophe
diff --git a/libstdc++-v3/include/experimental/io_context b/libstdc++-v3/include/experimental/io_context index c59f8c8e73b..c878d5a7025 100644 --- a/libstdc++-v3/include/experimental/io_context +++ b/libstdc++-v3/include/experimental/io_context @@ -562,7 +562,11 @@ inline namespace v1 } }; +#ifdef _GLIBCXX_HAS_GTHREADS atomic<count_type> _M_work_count; +#else + count_type _M_work_count; +#endif mutable execution_context::mutex_type _M_mtx; queue<function<void()>> _M_op; bool _M_stopped = false; diff --git a/libstdc++-v3/src/c++17/memory_resource.cc b/libstdc++-v3/src/c++17/memory_resource.cc index c0c7cf0cf83..63856eadaf5 100644 --- a/libstdc++-v3/src/c++17/memory_resource.cc +++ b/libstdc++-v3/src/c++17/memory_resource.cc @@ -27,9 +27,9 @@ #include <atomic> #include <bit> // has_single_bit, bit_ceil, bit_width #include <new> +#include <bits/move.h> // std::__exchange #if ATOMIC_POINTER_LOCK_FREE != 2 # include <bits/std_mutex.h> // std::mutex, std::lock_guard -# include <bits/move.h> // std::__exchange #endif #if __has_cpp_attribute(clang::require_constant_initialization) @@ -94,10 +94,31 @@ namespace pmr __constinit constant_init<newdel_res_t> newdel_res{}; __constinit constant_init<null_res_t> null_res{}; -#if ATOMIC_POINTER_LOCK_FREE == 2 + +#ifndef _GLIBCXX_HAS_GTHREADS +# define _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED + // Single-threaded, no need for synchronization + struct atomic_mem_res + { + constexpr + atomic_mem_res(memory_resource* r) : val(r) { } + + memory_resource* val; + + memory_resource* load(std::memory_order) const + { + return val; + } + + memory_resource* exchange(memory_resource* r, std::memory_order) + { + return std::__exchange(val, r); + } + }; +#elif ATOMIC_POINTER_LOCK_FREE == 2 using atomic_mem_res = atomic<memory_resource*>; # define _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED -#elif defined(_GLIBCXX_HAS_GTHREADS) +#else // Can't use pointer-width atomics, define a type using a mutex instead: struct atomic_mem_res { @@ -123,27 +144,7 @@ namespace pmr return std::__exchange(val, r); } }; -#else -# define _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED - // Single-threaded, no need for synchronization - struct atomic_mem_res - { - constexpr - atomic_mem_res(memory_resource* r) : val(r) { } - - memory_resource* val; - - memory_resource* load(std::memory_order) const - { - return val; - } - - memory_resource* exchange(memory_resource* r, std::memory_order) - { - return std::__exchange(val, r); - } - }; -#endif // ATOMIC_POINTER_LOCK_FREE == 2 +#endif #ifdef _GLIBCXX_ATOMIC_MEM_RES_CAN_BE_CONSTANT_INITIALIZED __constinit constant_init<atomic_mem_res> default_res{&newdel_res.obj};