diff mbox

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

Message ID 20150331134136.GS9755@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely March 31, 2015, 1:41 p.m. UTC 
On 26/03/15 13:21 +0000, Jonathan Wakely wrote:
>This includes your fix to avoid decreasing alignment, but I didn't add
>a test for that as I couldn't make it fail on any of the targets I
>test on.

>commit f796769ad20c0353490b9f1a7e019e2f0c1771fb
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Wed Sep 3 15:39:53 2014 +0100
>
>    	PR libstdc++/62259
>    	PR libstdc++/65147
>    	* include/std/atomic (atomic<T>): Increase alignment for types with
>    	the same size as one of the integral types.
>    	* testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
>    	* testsuite/29_atomics/atomic/62259.cc: New.

My patch was not sufficient to fix 65147, because I didn't increase
the alignment of the std::atomic<integral type> specializations, and
std::atomic<16-byte type> is only aligned correctly if __int128 is
supported, which isn't true on x86 and other 32-bit targets.

This is the best I've come up with, does anyone have any better ideas
than the #else branch to hardcode alignment of 16-byte types to 16?

Comments

Richard Henderson March 31, 2015, 2:54 p.m. UTC | #1 
On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
> This is the best I've come up with, does anyone have any better ideas
> than the #else branch to hardcode alignment of 16-byte types to 16?

If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
am aware).  So we're forced to use a locking path anyway.


And if we do want the alignment, do we stop pretending with all the sizeof's
and alignof's and just use power-of-two size alignment for all of them, e.g.

  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)


r~
Jonathan Wakely March 31, 2015, 3:03 p.m. UTC | #2 
On 31/03/15 07:54 -0700, Richard Henderson wrote:
>On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
>> This is the best I've come up with, does anyone have any better ideas
>> than the #else branch to hardcode alignment of 16-byte types to 16?
>
>If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
>a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
>am aware).  So we're forced to use a locking path anyway.

The C front end gives struct S { char s[16]; } 16 byte alignment, and
I'd like std::atomic<S> and _Atomic struct S to be layout compatible,
although it's not essential (or required by any standard).

And it matters most for the integral types, not arbitrary structs.

>And if we do want the alignment, do we stop pretending with all the sizeof's
>and alignof's and just use power-of-two size alignment for all of them, e.g.
>
>  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)

Yeah, I wondered about that too. Joseph indicated there are targets
where C gives alignof(_Atomic X) != sizeof(X), which is why the target
hook exists, but maybe we can just not worry about those targets for
now.  For GCC 6 we can look into the new attribute Andrew did in the
atomics branch so that we can make std::atomic use the target hook
directly instead of trying to simulate its effects in C++ code.
Richard Henderson March 31, 2015, 3:13 p.m. UTC | #3 
On 03/31/2015 08:03 AM, Jonathan Wakely wrote:
> On 31/03/15 07:54 -0700, Richard Henderson wrote:
>> On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
>>> This is the best I've come up with, does anyone have any better ideas
>>> than the #else branch to hardcode alignment of 16-byte types to 16?
>>
>> If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
>> a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
>> am aware).  So we're forced to use a locking path anyway.
> 
> The C front end gives struct S { char s[16]; } 16 byte alignment...

Um, I'm pretty sure it doesn't.

	struct S { char s[16]; };
	int x = __alignof(struct S);

	.type	x, @object
	.size	x, 4
x:
	.long	1

What you're interpreting as alignment for that struct is probably optional
*additional* alignment from LOCAL_ALIGNMENT or LOCAL_DECL_ALIGNMENT or something.

> And it matters most for the integral types, not arbitrary structs.
> 
>> And if we do want the alignment, do we stop pretending with all the sizeof's
>> and alignof's and just use power-of-two size alignment for all of them, e.g.
>>
>>  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)
> 
> Yeah, I wondered about that too. Joseph indicated there are targets
> where C gives alignof(_Atomic X) != sizeof(X), which is why the target
> hook exists, but maybe we can just not worry about those targets for
> now.

Those targets have alignof < sizeof.  So in a way we'd probably be doing them a
favor.  I know for instance that CRIS is in this category, where most data is
all byte aligned, but atomics must be naturally aligned.

And, as you note, just so long as we do the same thing for _Atomic once we
get that merged.


r~
Jonathan Wakely March 31, 2015, 3:40 p.m. UTC | #4 
On 31/03/15 08:13 -0700, Richard Henderson wrote:
>On 03/31/2015 08:03 AM, Jonathan Wakely wrote:
>> On 31/03/15 07:54 -0700, Richard Henderson wrote:
>>> On 03/31/2015 06:41 AM, Jonathan Wakely wrote:
>>>> This is the best I've come up with, does anyone have any better ideas
>>>> than the #else branch to hardcode alignment of 16-byte types to 16?
>>>
>>> If there's no 16 byte type, are we convinced this matters?  I mean, there isn't
>>> a 16-byte atomic instruction for 32-bit x86 (or any other 32-bit cpu of which I
>>> am aware).  So we're forced to use a locking path anyway.
>>
>> The C front end gives struct S { char s[16]; } 16 byte alignment...
>
>Um, I'm pretty sure it doesn't.
>
>	struct S { char s[16]; };
>	int x = __alignof(struct S);
>
>	.type	x, @object
>	.size	x, 4
>x:
>	.long	1
>
>What you're interpreting as alignment for that struct is probably optional
>*additional* alignment from LOCAL_ALIGNMENT or LOCAL_DECL_ALIGNMENT or something.

Sorry for not being clear, I meant __alignof(_Atomic struct S) is 16.

>> And it matters most for the integral types, not arbitrary structs.
>>
>>> And if we do want the alignment, do we stop pretending with all the sizeof's
>>> and alignof's and just use power-of-two size alignment for all of them, e.g.
>>>
>>>  min_align = ((size & (size - 1)) || size > 16 ? 0 : size)
>>
>> Yeah, I wondered about that too. Joseph indicated there are targets
>> where C gives alignof(_Atomic X) != sizeof(X), which is why the target
>> hook exists, but maybe we can just not worry about those targets for
>> now.
>
>Those targets have alignof < sizeof.  So in a way we'd probably be doing them a
>favor.  I know for instance that CRIS is in this category, where most data is
>all byte aligned, but atomics must be naturally aligned.

Aha, I wondered why CRIS overrides the atomic_align_for_mode hook when
it seemed to be giving them natural alignment anyway - I didn't
realise non-atomic types are only byte-aligned.
diff mbox

Patch

commit d0ccfb0523066c69f3d22d9cdd617a139c57f9e1
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Mar 30 14:28:01 2015 +0100

    	PR libstdc++/65147
    	* include/bits/atomic_base.h (__atomic_base): Align as underlying
    	type.
    	* include/std/atomic (atomic<T>): Hardcode alignment for 16-byte
    	types when __int128 is not available.
    	* testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number.
    	* testsuite/29_atomics/atomic/65147.cc: New.

diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h
index 8104c98..48931ac 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -235,7 +235,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // 8 bytes, since that is what GCC built-in functions for atomic
   // memory access expect.
   template<typename _ITp>
-    struct __atomic_base
+    struct alignas(_ITp) __atomic_base
     {
     private:
       typedef _ITp 	__int_type;
@@ -559,7 +559,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   /// Partial specialization for pointer types.
   template<typename _PTp>
-    struct __atomic_base<_PTp*>
+    struct alignas(_PTp*) __atomic_base<_PTp*>
     {
     private:
       typedef _PTp* 	__pointer_type;
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index 88c8b17..2b09477 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -175,6 +175,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	: sizeof(_Tp) == sizeof(long long) ? alignof(long long)
 #ifdef _GLIBCXX_USE_INT128
 	: sizeof(_Tp) == sizeof(__int128)  ? alignof(__int128)
+#else
+	: sizeof(_Tp) == 16  ? 16
 #endif
 	: 0;
 
diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
index 6f618a0..f755be0 100644
--- a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc
@@ -27,4 +27,4 @@  struct X {
   char stuff[0]; // GNU extension, type has zero size
 };
 
-std::atomic<X> a;  // { dg-error "not supported" "" { target *-*-* } 189 }
+std::atomic<X> a;  // { dg-error "not supported" "" { target *-*-* } 191 }
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..bb92513
--- /dev/null
+++ b/libstdc++-v3/testsuite/29_atomics/atomic/65147.cc
@@ -0,0 +1,42 @@ 
+// 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 }
+
+// PR libstdc++65147
+
+#include <atomic>
+
+static_assert( alignof(std::atomic<short>) == alignof(short),
+    "atomic short must be aligned like short" );
+
+static_assert( alignof(std::atomic<int>) == alignof(int),
+    "atomic int must be aligned like int" );
+
+static_assert( alignof(std::atomic<long>) == alignof(long),
+    "atomic long must be aligned like long" );
+
+static_assert( alignof(std::atomic<long long>) == alignof(long long),
+    "atomic long long must be aligned like long long" );
+
+struct S {
+  char s[16];
+};
+
+static_assert( alignof(std::atomic<S>) > 1,
+    "atomic 16-byte struct must not be aligned like char" );