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

[Jonathan Wakely <jwakely at redhat dot com>]

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