[Patch libstdc++] Rewrite cpu/generic/atomic_word.h
Ramana Radhakrishnan
ramana.radhakrishnan@foss.arm.com
Tue Jun 9 09:50:00 GMT 2015
On 22/05/15 17:56, Torvald Riegel wrote:
> On Fri, 2015-05-22 at 12:37 +0100, Ramana Radhakrishnan wrote:
>> Hi,
>>
>> While writing atomic_word.h for the ARM backend to fix PR target/66200
>> I
>> thought it would make more sense to write it all up with atomic
>> primitives instead of providing various fragile bits of inline
>> asssembler. Thus this patch came about. I intend to ask for a
>> specialized version of this to be applied for the ARM backend in any
>> case after testing completes.
>>
>> If this goes in as a cleanup I expect all the ports to be deleting
>> their
>> atomic_word.h implementation in the various ports directories and
>> I'll
>> follow up with patches asking port maintainers to test and apply for
>> each of the individual cases if this is deemed to be good enough.
>
> Could you use C++11 atomics right away instead of adapting the wrappers?
I spent some time trying to massage guard.cc into using C++11 atomics
but it was just easier to write it with the builtins - I consider this
to be a step improvement compared to where we are currently.
Rewritten to use the builtins in guard.cc instead of std::atomic as that
appears to be a bigger project than moving forward compared to where we
are. I prefer small steps rather than big steps in these areas. Further
I do not believe you can use the C++11 language features in the headers
as they were not necessarily part of the standard for tr1 etc. and thus
it's better to remain with the builtins, after all I am also continuing
with existing practice in the headers.
>
> I think the more non-C++11 atomic operations/wrappers we can remove the
> better.
>
>> diff --git a/libstdc++-v3/config/cpu/generic/atomic_word.h b/libstdc
>> ++-v3/config/cpu/generic/atomic_word.h
>> index 19038bb..bedce31 100644
>> --- a/libstdc++-v3/config/cpu/generic/atomic_word.h
>> +++ b/libstdc++-v3/config/cpu/generic/atomic_word.h
>> @@ -29,19 +29,46 @@
>> #ifndef _GLIBCXX_ATOMIC_WORD_H
>> #define _GLIBCXX_ATOMIC_WORD_H 1
>>
>> +#include <bits/cxxabi_tweaks.h>
>> +
>> typedef int _Atomic_word;
>>
>> -// Define these two macros using the appropriate memory barrier for
>> the target.
>> -// The commented out versions below are the defaults.
>> -// See ia64/atomic_word.h for an alternative approach.
>> +
>> +namespace __gnu_cxx _GLIBCXX_VISIBILITY(default)
>> +{
>> + // Test the first byte of __g and ensure that no loads are hoisted
>> across
>> + // the test.
>
> That comment is not quite correct. I'd just say that this is a
> memory_order_acquire load and a comparison.
>
Done.
>> + inline bool
>> + __test_and_acquire (__cxxabiv1::__guard *__g)
>> + {
>> + unsigned char __c;
>> + unsigned char *__p = reinterpret_cast<unsigned char *>(__g);
>> + __atomic_load (__p, &__c, __ATOMIC_ACQUIRE);
>> + return __c != 0;
>> + }
>> +
>> + // Set the first byte of __g to 1 and ensure that no stores are
>> sunk
>> + // across the store.
>
> Likewise; I'd just say this is a memory_order_release store with 1 as
> value.
Ok. I've now realized that this is superfluous and better to fix the one
definition in guard.cc to do the right thing instead of something like this.
>
>> + inline void
>> + __set_and_release (__cxxabiv1::__guard *__g)
>> + {
>> + unsigned char *__p = reinterpret_cast<unsigned char *>(__g);
>> + unsigned char val = 1;
>> + __atomic_store (__p, &val, __ATOMIC_RELEASE);
>> + }
>> +
>> +#define _GLIBCXX_GUARD_TEST_AND_ACQUIRE(G)
>> __gnu_cxx::__test_and_acquire (G)
>> +#define _GLIBCXX_GUARD_SET_AND_RELEASE(G)
>> __gnu_cxx::__set_and_release (G)
>>
>> // This one prevents loads from being hoisted across the barrier;
>> // in other words, this is a Load-Load acquire barrier.
>
> Likewise; I'd just say that this is an mo_acquire fence.
>
>> -// This is necessary iff TARGET_RELAXED_ORDERING is defined in
>> tm.h.
>> -// #define _GLIBCXX_READ_MEM_BARRIER __asm __volatile ("":::"memory")
>> +// This is necessary iff TARGET_RELAXED_ORDERING is defined in tm.h.
>> +#define _GLIBCXX_READ_MEM_BARRIER __atomic_thread_fence
>> (__ATOMIC_ACQUIRE)
>>
>> // This one prevents stores from being sunk across the barrier; in
>> other
>> // words, a Store-Store release barrier.
>
> Likewise; mo_release fence.
>
>> -// #define _GLIBCXX_WRITE_MEM_BARRIER __asm __volatile
>> ("":::"memory")
>> +#define _GLIBCXX_WRITE_MEM_BARRIER __atomic_thread_fence
>> (__ATOMIC_RELEASE)
>> +
>> +}
I don't think we can remove _GLIBCXX_READ_MEM_BARRIER and
_GLIBCXX_WRITE_MEM_BARRIER from atomic_word.h even though they are
superseded by the atomics as it is published in the documentation as
available macros.
It was obvious that alpha, powerpc, aix, ia64 could just fall back to
the standard implementations. The cris and sparc implementations have
different types for _Atomic_word from generic and the rest - my guess is
sparc can remove the _GLIBCXX_READ_MEM_BARRIER and
_GLIBCXX_WRITE_MEM_BARRIER but I have no way of testing this is sane.
I'll leave that to the respective target maintainers for sparc and cris
to deal as appropriate.
2015-06-09 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
PR c++/66192
PR target/66200
* config/cpu/generic/atomic_word.h (_GLIBCXX_READ_MEM_BARRIER):
Define
(_GLIBCXX_WRITE_MEM_BARRIER): Likewise
* include/bits/shared_ptr_base.h: Use ACQ_REL barrier.
* include/ext/atomicity.h: Likewise.
* include/tr1/shared_ptr.h: Likewise.
* libsupc++/guard.cc (__test_and_acquire): Rewrite with atomics.
Update comment.
(__set_and_release): Likewise.
* testsuite/20_util/shared_ptr/cons/43820_neg.cc (void test01): Adjust
for line numbers.
* testsuite/20_util/shared_ptr/cons/void_neg.cc: Likewise.
* testsuite/tr1/2_general_utilities/shared_ptr/cons/43820_neg.cc: Likewise.
2015-06-09 Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
* config/cpu/alpha/atomic_word.h: Remove.
* config/cpu/ia64/atomic_word.h: Remove.
* config/cpu/powerpc/atomic_word.h: Remove.
* config/os/aix/atomic_word.h: Remove.
* configure.host (atomic_word_dir) [ia64, aix*, powerpc, alpha]:
Use generic definition.
Bootstrap and regression tested on aarch64-none-linux-gnu,
arm-none-linux-gnueabihf and arm-none-eabi.
regards
Ramana
>>
>> #endif
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libstdc-TLC-for-guards-and-atomic-operations.patch
Type: text/x-diff
Size: 8672 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20150609/d9a30998/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-libstdc-TLC-Ports-PowerPC-Alpha-AIX-IA64-Use-generic.patch
Type: text/x-diff
Size: 10495 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20150609/d9a30998/attachment-0001.bin>
More information about the Libstdc++
mailing list