From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6260 invoked by alias); 11 Jun 2015 15:36:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 6240 invoked by uid 89); 11 Jun 2015 15:36:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-ig0-f176.google.com Received: from mail-ig0-f176.google.com (HELO mail-ig0-f176.google.com) (209.85.213.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 11 Jun 2015 15:36:09 +0000 Received: by igblz2 with SMTP id lz2so7926636igb.1; Thu, 11 Jun 2015 08:36:07 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.42.83.212 with SMTP id i20mr11314048icl.91.1434036967062; Thu, 11 Jun 2015 08:36:07 -0700 (PDT) Received: by 10.36.108.21 with HTTP; Thu, 11 Jun 2015 08:36:07 -0700 (PDT) In-Reply-To: <5576B6D5.5010209@foss.arm.com> References: <555F14F1.2090504@foss.arm.com> <1432313791.3077.8.camel@triegel.csb> <5576B6D5.5010209@foss.arm.com> Date: Thu, 11 Jun 2015 15:36:00 -0000 Message-ID: Subject: Re: [Patch libstdc++] Rewrite cpu/generic/atomic_word.h From: David Edelsohn To: Ramana Radhakrishnan Cc: Torvald Riegel , "gcc-patches@gcc.gnu.org" , "libstdc++@gcc.gnu.org" , Richard Henderson , hp@axis.com, Steve Ellcey , Jim Wilson Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-06/txt/msg00856.txt.bz2 On Tue, Jun 9, 2015 at 5:50 AM, Ramana Radhakrishnan 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 >>> + >>> 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(__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(__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 > > 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 > > * 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