From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46355 invoked by alias); 9 Apr 2015 11:17:39 -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 46337 invoked by uid 89); 9 Apr 2015 11:17:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham 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; Thu, 09 Apr 2015 11:17:37 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 2EE258EA5D; Thu, 9 Apr 2015 11:17:36 +0000 (UTC) Received: from localhost (ovpn-116-20.ams2.redhat.com [10.36.116.20]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t39BHYsb015907; Thu, 9 Apr 2015 07:17:35 -0400 Date: Thu, 09 Apr 2015 11:17:00 -0000 From: Jonathan Wakely To: Richard Henderson Cc: Hans-Peter Nilsson , libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [libstdc++/65033] Give alignment info to libatomic Message-ID: <20150409111734.GS9755@redhat.com> References: <54DD19B7.6060401@redhat.com> <20150403141333.GY9755@redhat.com> <551EE648.50302@redhat.com> <20150407131408.GC9755@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20150407131408.GC9755@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2015-04/txt/msg00379.txt.bz2 On 07/04/15 14:14 +0100, Jonathan Wakely wrote: >On 03/04/15 12:13 -0700, Richard Henderson wrote: >>On 04/03/2015 07:13 AM, Jonathan Wakely wrote: >>>+++ 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); >>>+ >> >>This doesn't work for non-power-of-two sized _Tp. >> >>Again, we don't have *any* target for which alignof(integer) > sizeof(integer). >>So if you care about forcing natural alignment, don't bother with the alignof >>on the integrals, as you're doing with _S_int_alignment at the moment. > >OK, the attached patch uses the simpler version you proposed, so >integral types and non-integral types with size 1/2/4/8/16 are aligned >to at least their size. Committed to trunk. > >What about the __atomic_base<_PTp*> partial specialization for >pointers, do we need to use alignas on its data member, or are >pointers always aligned appropriately on all targets? > >And what about these lines: > > void *__a = reinterpret_cast(-__alignof(_M_i)); > return __atomic_is_lock_free(sizeof(_M_i), __a); > >Do we still need that if we use alignas on the data members? >If we do, do you agree with HP that it should use _S_alignment not >__alignof(_M_i)? That seems redundant to me once _M_i has been given a >fixed alignment with alignas. > >commit 18fe6cb4dd74e5e5e4586ec10adba997e2e28c81 >Author: Jonathan Wakely >Date: Fri Apr 3 15:14:40 2015 +0100 > > 2015-04-07 Jonathan Wakely > Richard Henderson > > PR libstdc++/65147 > * include/bits/atomic_base.h (__atomic_base<_ITp>): Increase > alignment. > * include/std/atomic (atomic): For types with a power of two size set > alignment to at least the size. > * testsuite/29_atomics/atomic/65147.cc: New. > * testsuite/29_atomics/atomic_integral/65147.cc: New. > >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..125e37a 100644 >--- a/libstdc++-v3/include/std/atomic >+++ b/libstdc++-v3/include/std/atomic >@@ -165,18 +165,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > struct atomic > { > private: >- // Align 1/2/4/8/16-byte types the same as integer types of that size. >- // This matches the alignment effects of the C11 _Atomic qualifier. >+ // Align 1/2/4/8/16-byte types to at least their size. > 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) >-#ifdef _GLIBCXX_USE_INT128 >- : sizeof(_Tp) == sizeof(__int128) ? alignof(__int128) >-#endif >- : 0; >+ = (sizeof(_Tp) & (sizeof(_Tp) - 1)) || sizeof(_Tp) > 16 >+ ? 0 : sizeof(_Tp); > > static constexpr int _S_alignment > = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp); >diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc >new file mode 100644 >index 0000000..e05ec17 >--- /dev/null >+++ b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc >@@ -0,0 +1,28 @@ >+// Copyright (C) 2015 Free Software Foundation, Inc. >+// >+// This file is part of the GNU ISO C++ Library. This library is free >+// software; you can redistribute it and/or modify it under the >+// terms of the GNU General Public License as published by the >+// Free Software Foundation; either version 3, or (at your option) >+// any later version. >+ >+// This library is distributed in the hope that it will be useful, >+// but WITHOUT ANY WARRANTY; without even the implied warranty of >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+// GNU General Public License for more details. >+ >+// You should have received a copy of the GNU General Public License along >+// with this library; see the file COPYING3. If not see >+// . >+ >+// { dg-options "-std=gnu++11" } >+// { dg-do compile } >+ >+#include >+ >+struct S16 { >+ char c[16]; >+}; >+ >+static_assert( alignof(std::atomic) >= 16, >+ "atomic must be aligned to at least its size" ); >diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc b/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc >new file mode 100644 >index 0000000..a5f5b74 >--- /dev/null >+++ b/libstdc++-v3/testsuite/29_atomics/atomic_integral/65147.cc >@@ -0,0 +1,32 @@ >+// Copyright (C) 2015 Free Software Foundation, Inc. >+// >+// This file is part of the GNU ISO C++ Library. This library is free >+// software; you can redistribute it and/or modify it under the >+// terms of the GNU General Public License as published by the >+// Free Software Foundation; either version 3, or (at your option) >+// any later version. >+ >+// This library is distributed in the hope that it will be useful, >+// but WITHOUT ANY WARRANTY; without even the implied warranty of >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+// GNU General Public License for more details. >+ >+// You should have received a copy of the GNU General Public License along >+// with this library; see the file COPYING3. If not see >+// . >+ >+// { dg-options "-std=gnu++11" } >+// { dg-do compile } >+ >+#include >+ >+static_assert( alignof(std::atomic) >= sizeof(char), >+ "atomic must be aligned to at least its size" ); >+static_assert( alignof(std::atomic) >= sizeof(short), >+ "atomic must be aligned to at least its size" ); >+static_assert( alignof(std::atomic) >= sizeof(int), >+ "atomic must be aligned to at least its size" ); >+static_assert( alignof(std::atomic) >= sizeof(long), >+ "atomic must be aligned to at least its size" ); >+static_assert( alignof(std::atomic) >= sizeof(long long), >+ "atomic must be aligned to at least its size" );