This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

Attachment: atomic-align.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]