From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68459 invoked by alias); 3 Apr 2015 14:13:38 -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 68439 invoked by uid 89); 3 Apr 2015 14:13:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 03 Apr 2015 14:13:36 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id A7FD58F30E; Fri, 3 Apr 2015 14:13:34 +0000 (UTC) Received: from localhost (ovpn-116-96.ams2.redhat.com [10.36.116.96]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t33EDXTv018061; Fri, 3 Apr 2015 10:13:34 -0400 Date: Fri, 03 Apr 2015 14:13:00 -0000 From: Jonathan Wakely To: Hans-Peter Nilsson Cc: Richard Henderson , libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [libstdc++/65033] Give alignment info to libatomic Message-ID: <20150403141333.GY9755@redhat.com> References: <54DD19B7.6060401@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="owPIoVL9FiqC4k42" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-04/txt/msg00121.txt.bz2 --owPIoVL9FiqC4k42 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline Content-length: 6042 On 03/04/15 05:24 -0400, Hans-Peter Nilsson wrote: >On Thu, 2 Apr 2015, Hans-Peter Nilsson wrote: >> Why then use __alignof(_M_i) (the object-alignment) >> instead of _S_alignment (the deduced alas insufficiently >> increased type-alignment)? Isn't the object aligned to _S_alignment? > >(The immediate reason is that _S_alignment wasn't there until a >later revision, but the gist of the question remains. :-) >> > making sure that atomic_is_lock_free returns the same >> > value for all objects of a given type, >> >> (That would work but it doesn't seem to be the case.) Because we haven't done anything to increase the alignment for the __atomic_base<_ITp> specialization yet, see the additional patch at: https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01664.html (Although that's insufficient as you point out, because it should depend on the size too.) >> > we probably should have changed the >> > interface so that we would pass size and alignment rather than size and object >> > pointer. >> > >> > Instead, we decided that passing null for the object pointer would be >> > sufficient. But as this PR shows, we really do need to take alignment into >> > account. >> >> Regarding what's actually needed, alignment of an atomic type >> should always be *forced to be at least the natural alignment of >> of the object* (with non-power-of-two sized-objects rounded up) >> and until then atomic types won't work for targets where the >> non-atomic equivalents have less alignment (as straddling a >> page-boundary won't be lock-less-atomic anywhere where >> straddling a page-boundary may cause a non-atomic-access...) So, >> not target-specific except for targets that require even >> higher-than-natural alignment. > >So, can we do something like this instead for gcc5? >(Completely untested and may be syntactically, namespacingly and >cxxstandardversionly flawed.) > >Index: include/std/atomic >=================================================================== >--- include/std/atomic (revision 221849) >+++ include/std/atomic (working copy) >@@ -165,16 +165,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > struct atomic > { > private: >- // Align 1/2/4/8/16-byte types the same as integer types of that size. >+ // Align 1/2/4/8/16-byte types to the natural alignment of that size. > // This matches the alignment effects of the C11 _Atomic qualifier. > static constexpr int _S_min_alignment >- = sizeof(_Tp) == sizeof(char) ? alignof(char) >- : sizeof(_Tp) == sizeof(short) ? alignof(short) >- : sizeof(_Tp) == sizeof(int) ? alignof(int) >- : sizeof(_Tp) == sizeof(long) ? alignof(long) >- : sizeof(_Tp) == sizeof(long long) ? alignof(long long) >+ = sizeof(_Tp) == sizeof(char) ? max(sizeof(char), alignof(char)) >+ : sizeof(_Tp) == sizeof(short) ? max(sizeof(short), alignof(short)) >+ : sizeof(_Tp) == sizeof(int) ? max(sizeof(int), alignof(int)) >+ : sizeof(_Tp) == sizeof(long) ? max(sizeof(long), alignof(long)) >+ : sizeof(_Tp) == sizeof(long long) ? max(sizeof(long long), alignof(long long)) > #ifdef _GLIBCXX_USE_INT128 >- : sizeof(_Tp) == sizeof(__int128) ? alignof(__int128) >+ : sizeof(_Tp) == sizeof(__int128) ? max(sizeof(__int128), alignof(__int128)) > #endif > : 0; Instead of changing every case in the condition to include sizeof why not just do it afterwards using sizeof(_Tp), in the _S_alignment calculation? We know sizeof(_Tp) == sizeof(corresponding integer type) because that's the whole point of the conditionals! See attachment. >@@ -216,7 +216,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > is_lock_free() const noexcept > { > // Produce a fake, minimally aligned pointer. >- void *__a = reinterpret_cast(-__alignof(_M_i)); >+ void *__a = reinterpret_cast(-_S_alignment); > return __atomic_is_lock_free(sizeof(_M_i), __a); > } If _M_i is aligned to _S_alignment then what difference does the change above make? It doesn't matter if the value is per-object if we've forced all such objects to have the same alignment, does it? Or is it different if a std::atomic is included in some other struct and the user forces a different alignment on it? I don't think we really need to support that, users shouldn't be doing that. >Index: include/bits/atomic_base.h >=================================================================== >--- include/bits/atomic_base.h (revision 221849) >+++ include/bits/atomic_base.h (working copy) >@@ -240,7 +240,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > private: > typedef _ITp __int_type; > >- __int_type _M_i; >+ // Align 1/2/4/8/16-byte types to the natural alignment of that size. >+ // This matches the alignment effects of the C11 _Atomic qualifier. >+ static constexpr int _S_min_alignment >+ = sizeof(_Tp) == sizeof(char) ? max(sizeof(char), __alignof(char)) >+ : sizeof(_Tp) == sizeof(short) ? max(sizeof(short), __alignof(short)) >+ : sizeof(_Tp) == sizeof(int) ? max(sizeof(int), __alignof(int)) >+ : sizeof(_Tp) == sizeof(long) ? max(sizeof(long), __alignof(long)) >+ : sizeof(_Tp) == sizeof(long long) ? max(sizeof(long long), __alignof(long long)) >+#ifdef _GLIBCXX_USE_INT128 >+ : sizeof(_Tp) == sizeof(__int128) ? max(sizeof(__int128), __alignof(__int128)) >+#endif >+ : 0; >+ >+ static constexpr int _S_alignment >+ = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : __alignof(_Tp); >+ >+ __int_type _M_i __attribute ((__aligned(_S_alignment))); This is massively overcomplicated. Unlike the generic template in that handles arbitrary types, here in we know for a fact that _ITp is one of the standard integer types so we don't need to do the silly dance to find a standard integer type of the same size. The attached patch against trunk should have the same result with much less effort. It doesn't include the changes to the reinterpret_cast expressions to produce a minimally aligned pointer, but I think this is progress, thanks :-) --owPIoVL9FiqC4k42 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="atomic-align.txt" Content-length: 1612 diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h index 8104c98..79769cf 100644 --- a/libstdc++-v3/include/bits/atomic_base.h +++ b/libstdc++-v3/include/bits/atomic_base.h @@ -240,7 +240,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION private: typedef _ITp __int_type; - __int_type _M_i; + static constexpr int _S_alignment = + sizeof(_ITp) > alignof(_ITp) ? sizeof(_ITp) : alignof(_ITp); + + alignas(_S_alignment) __int_type _M_i; public: __atomic_base() noexcept = default; diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic index 88c8b17..81f29d8 100644 --- a/libstdc++-v3/include/std/atomic +++ b/libstdc++-v3/include/std/atomic @@ -165,9 +165,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct atomic { private: - // Align 1/2/4/8/16-byte types the same as integer types of that size. + // Align 1/2/4/8/16-byte types the natural alignment of that size. // This matches the alignment effects of the C11 _Atomic qualifier. - static constexpr int _S_min_alignment + static constexpr int _S_int_alignment = sizeof(_Tp) == sizeof(char) ? alignof(char) : sizeof(_Tp) == sizeof(short) ? alignof(short) : sizeof(_Tp) == sizeof(int) ? alignof(int) @@ -178,6 +178,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #endif : 0; + static constexpr int _S_min_alignment + = _S_int_alignment > sizeof(_Tp) ? _S_int_alignment : sizeof(_Tp); + static constexpr int _S_alignment = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp); --owPIoVL9FiqC4k42--