[libstdc++/65033] Give alignment info to libatomic

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.

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<void *>(-__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.

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<void *>(-__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 <jwakely@redhat.com>
>Date:   Fri Apr 3 15:14:40 2015 +0100
>
>    2015-04-07  Jonathan Wakely  <jwakely@redhat.com>
>    	    Richard Henderson  <rth@redhat.com>
>    
>    	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
>+// <http://www.gnu.org/licenses/>.
>+
>+// { dg-options "-std=gnu++11" }
>+// { dg-do compile }
>+
>+#include <atomic>
>+
>+struct S16 {
>+   char c[16];
>+};
>+
>+static_assert( alignof(std::atomic<S16>) >= 16,
>+    "atomic<S16> 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
>+// <http://www.gnu.org/licenses/>.
>+
>+// { dg-options "-std=gnu++11" }
>+// { dg-do compile }
>+
>+#include <atomic>
>+
>+static_assert( alignof(std::atomic<char>) >= sizeof(char),
>+    "atomic<char> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<short>) >= sizeof(short),
>+    "atomic<short> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<int>) >= sizeof(int),
>+    "atomic<int> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<long>) >= sizeof(long),
>+    "atomic<long> must be aligned to at least its size" );
>+static_assert( alignof(std::atomic<long long>) >= sizeof(long long),
>+    "atomic<long long> must be aligned to at least its size" );