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

Fri Apr 3 14:13:00 GMT 2015

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<void *>(-__alignof(_M_i));
>+	void *__a = reinterpret_cast<void *>(-_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<T> 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
<atomic> that handles arbitrary types, here in <bits/atomic_base.h> 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<void *>
expressions to produce a minimally aligned pointer, but I think this
is progress, thanks :-)

-------------- next part --------------

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);
 
