Re: [Patch libstdc++] Rewrite cpu/generic/atomic_word.h

[David Edelsohn <dje dot gcc at gmail dot com>]

On Tue, Jun 9, 2015 at 5:50 AM, Ramana Radhakrishnan
<ramana.radhakrishnan@foss.arm.com> wrote:
>
>
> 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.

The revised patch looks good on PowerPC.

Thanks, David