diff mbox

Implement C11 _Atomic

Message ID 528E2CB6.7020104@redhat.com
State New
Headers show

Commit Message

Andrew MacLeod Nov. 21, 2013, 3:54 p.m. UTC
On 11/21/2013 10:20 AM, Hans-Peter Nilsson wrote:
> On Thu, 21 Nov 2013, Andrew MacLeod wrote:
>>> Or is that part also required for
>>> anything-other-than-ordinary-C-type alignment for the target;
>>> say, natural 4-byte alignment of 4-byte-types for targets where
>>> alignment is otherwise "packed"; where only 1-byte alignment of
>>> the basic type is ABI-mandated?
>>>
>> If I understand correctly, yes, it would be needed there as well.... if the
>> compiler thinks alignof (int) is 1, then I believe it will set alignof(_Atomic
>> int) to 1 as well, and that's probably not good.
> Right.
>
>> Basically, atomic_types are given their own type nodes in tree.c:
>>    atomicQI_type_node = build_atomic_base (unsigned_intQI_type_node);
>>    atomicHI_type_node = build_atomic_base (unsigned_intHI_type_node);
>>    atomicSI_type_node = build_atomic_base (unsigned_intSI_type_node);
>>    atomicDI_type_node = build_atomic_base (unsigned_intDI_type_node);
>>    atomicTI_type_node = build_atomic_base (unsigned_intTI_type_node);
> It sounds like I should be able to hack that from the port in
> some other tree initializer hook?
>
> I'm trying to avoid ABI breakage of course.  I'd rather not have
> to ask people not to use _Atomic with 4.9 for CRIS ports using
> official releases or have ABI breakage with the next release.
> Maybe there's one other port in the same situation...

None I am aware of, but that doesn't mean much :-)
>> and on the branch that code instead looks something like:
>>
>> #define SET_ATOMIC_TYPE_NODE(TYPE, MODE, DEFAULT)               \
>>   (TYPE) = build_atomic_base (DEFAULT, targetm.atomic_align_for_mode (MODE));
>>
>>    SET_ATOMIC_TYPE_NODE (atomicQI_type_node, QImode, unsigned_intQI_type_node);
>> <...>
>>
>> which provides a target hook to override the default values and a target can
>> set them to whatever it deems necessary.
> Yah, that'd be nice.  Doesn't sound like more than the target
> hook and the patched lines above left for that to happen,
> though?  Or perhaps that's a too-naive assumption. I guess I
> should have a look.
>
That's all that is in theory needed, but it hasnt been tested very well.

>> There was insufficient time to test and fully flush this out, so it hasn't
>> made it into mainline.  Its only thanks to Josephs heroic efforts we have C11
>> :-)
>>
>> I don't think its a lot of code if you wanted to fool with it for your port.
> So, what would be needed in terms of testing and coding to get
> that part into 4.9?
>
>

If we add the hook for atomic_align_for_mode, and change the initalizers 
in tree.c, any target which doesnt need/use the hook should be 
unaffected. So everything remains as it is today.

So Putting the hook in shouldn't be an issue.  Then you can experiment 
with it on your port and see if you get the desired effect...

I've attached what I think the remaining bits are regarding the 
alignment.    All untested since I wrote it of course :-)  In any case,  
you should just need to provide a function to override the alignment for 
whatever mode(s)  you need with this...

I can bootstrap and check this on x86 to make sure it doesnt affect 
anything, and you can fool with it and see if you can get your desired 
results with your port.

Andrew

Comments

Joseph Myers Nov. 21, 2013, 4:32 p.m. UTC | #1
On Thu, 21 Nov 2013, Andrew MacLeod wrote:

> > I'm trying to avoid ABI breakage of course.  I'd rather not have
> > to ask people not to use _Atomic with 4.9 for CRIS ports using
> > official releases or have ABI breakage with the next release.
> > Maybe there's one other port in the same situation...
> 
> None I am aware of, but that doesn't mean much :-)

It has been suggested that hppa may have interesting issues for atomics - 
but I'd think atomics support for it in GCC would mainly be for GNU/Linux, 
where you have kernel helpers that may avoid the issues with the 
underlying architecture.

Note that if you want libstdc++ atomics to be ABI compatible with C 
atomics then there may be more work to do on that side (again, there are 
pieces on the branch).
Andrew MacLeod Nov. 21, 2013, 4:49 p.m. UTC | #2
On 11/21/2013 11:32 AM, Joseph S. Myers wrote:
> On Thu, 21 Nov 2013, Andrew MacLeod wrote:
>
>>> I'm trying to avoid ABI breakage of course.  I'd rather not have
>>> to ask people not to use _Atomic with 4.9 for CRIS ports using
>>> official releases or have ABI breakage with the next release.
>>> Maybe there's one other port in the same situation...
>> None I am aware of, but that doesn't mean much :-)
> It has been suggested that hppa may have interesting issues for atomics -
> but I'd think atomics support for it in GCC would mainly be for GNU/Linux,
> where you have kernel helpers that may avoid the issues with the
> underlying architecture.
>
> Note that if you want libstdc++ atomics to be ABI compatible with C
> atomics then there may be more work to do on that side (again, there are
> pieces on the branch).
>
I was just about to point out that  :-)    that would be a 
shortcoming... 4.9 C++11 atomics do not use the same mechanism yet, so 
they would still behave the same as 4.8 C++ atomics, even with the 
override present.... C11 _Atomic variables would be the only beneficiary 
at this point of the override.

Andrew
Hans-Peter Nilsson Nov. 21, 2013, 5:21 p.m. UTC | #3
On Thu, 21 Nov 2013, Andrew MacLeod wrote:
> If we add the hook for atomic_align_for_mode, and change the initalizers in
> tree.c, any target which doesnt need/use the hook should be unaffected. So
> everything remains as it is today.
>
> So Putting the hook in shouldn't be an issue.  Then you can experiment with it
> on your port and see if you get the desired effect...
>
> I've attached what I think the remaining bits are regarding the alignment.

Right, that's about what I expected.  Nice.

Of course, I'd argue that a better default for the atomic
alignment is the max of the default alignment and the natural
alignment, since *you can't have atomic support without extra
machinery for misaligned data* - straddling a page or cache
boundary has dire consequences.

But then the patch would not have the nice property of
"obviously" being a NOP elsewhere.  Then again, where it isn't a
NOP, you have breakage anyway.  Bah.

> All untested since I wrote it of course :-)  In any case,  you should just
> need to provide a function to override the alignment for whatever mode(s)  you
> need with this...
>
> I can bootstrap and check this on x86 to make sure it doesnt affect anything,
> and you can fool with it and see if you can get your desired results with your
> port.

Thanks!  I'll play with it and get back.

brgds, H-P
diff mbox

Patch

Index: doc/tm.texi
===================================================================
*** doc/tm.texi	(revision 205220)
--- doc/tm.texi	(working copy)
*************** The support includes the assembler, link
*** 11500,11505 ****
--- 11500,11509 ----
  The default value of this hook is based on target's libc.
  @end deftypefn
  
+ @deftypefn {Target Hook} {unsigned int} TARGET_ATOMIC_ALIGN_FOR_MODE (enum machine_mode @var{mode})
+ If defined, this function returns an appropriate alignment in bits for an atomic object of machine_mode @var{mode}.  If 0 is returned then the default alignment for the specified mode is used. 
+ @end deftypefn
+ 
  @deftypefn {Target Hook} void TARGET_ATOMIC_ASSIGN_EXPAND_FENV (tree *@var{hold}, tree *@var{clear}, tree *@var{update})
  ISO C11 requires atomic compound assignments that may raise floating-point exceptions to raise exceptions corresponding to the arithmetic operation whose result was successfully stored in a compare-and-exchange sequence.  This requires code equivalent to calls to @code{feholdexcept}, @code{feclearexcept} and @code{feupdateenv} to be generated at appropriate points in the compare-and-exchange sequence.  This hook should set @code{*@var{hold}} to an expression equivalent to the call to @code{feholdexcept}, @code{*@var{clear}} to an expression equivalent to the call to @code{feclearexcept} and @code{*@var{update}} to an expression equivalent to the call to @code{feupdateenv}.  The three expressions are @code{NULL_TREE} on entry to the hook and may be left as @code{NULL_TREE} if no code is required in a particular place.  The default implementation leaves all three expressions as @code{NULL_TREE}.  The @code{__atomic_feraiseexcept} function from @code{libatomic} may be of use as part of the code generated in @code{*@var{update}}.
  @end deftypefn
Index: doc/tm.texi.in
===================================================================
*** doc/tm.texi.in	(revision 205220)
--- doc/tm.texi.in	(working copy)
*************** and the associated definitions of those
*** 8407,8410 ****
--- 8407,8412 ----
  
  @hook TARGET_HAS_IFUNC_P
  
+ @hook TARGET_ATOMIC_ALIGN_FOR_MODE
+ 
  @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
Index: hooks.c
===================================================================
*** hooks.c	(revision 205220)
--- hooks.c	(working copy)
*************** hook_rtx_tree_int_null (tree a ATTRIBUTE
*** 358,363 ****
--- 358,370 ----
    return NULL;
  }
  
+ /* Generic hook that takes a machine mode and returns an unsigned int 0.  */
+ unsigned int
+ hook_uint_mode_0 (enum machine_mode m ATTRIBUTE_UNUSED)
+ {
+   return 0;
+ }
+ 
  /* Generic hook that takes three trees and returns the last one as is.  */
  tree
  hook_tree_tree_tree_tree_3rd_identity (tree a ATTRIBUTE_UNUSED,
Index: hooks.h
===================================================================
*** hooks.h	(revision 205220)
--- hooks.h	(working copy)
*************** extern tree hook_tree_tree_tree_tree_3rd
*** 92,97 ****
--- 92,98 ----
  extern tree hook_tree_tree_int_treep_bool_null (tree, int, tree *, bool);
  
  extern unsigned hook_uint_void_0 (void);
+ extern unsigned int hook_uint_mode_0 (enum machine_mode);
  
  extern bool default_can_output_mi_thunk_no_vcall (const_tree, HOST_WIDE_INT,
  						  HOST_WIDE_INT, const_tree);
Index: target.def
===================================================================
*** target.def	(revision 205220)
--- target.def	(working copy)
*************** DEFHOOKPOD
*** 5297,5302 ****
--- 5297,5313 ----
   @code{bool} @code{true}.",
   unsigned char, 1)
  
+ /* Return an unsigned int representing the alignment (in bits) of the atomic
+    type which maps to machine MODE.  This allows alignment to be overridden
+    as needed.  */
+ DEFHOOK
+ (atomic_align_for_mode,
+ "If defined, this function returns an appropriate alignment in bits for an\
+  atomic object of machine_mode @var{mode}.  If 0 is returned then the\
+  default alignment for the specified mode is used. ",
+  unsigned int, (enum machine_mode mode),
+  hook_uint_mode_0)
+ 
  DEFHOOK
  (atomic_assign_expand_fenv,
  "ISO C11 requires atomic compound assignments that may raise floating-point\
Index: tree.c
===================================================================
*** tree.c	(revision 205220)
--- tree.c	(working copy)
*************** make_or_reuse_accum_type (unsigned size,
*** 9547,9556 ****
     during initialization of data types to create the 5 basic atomic
     types. The generic build_variant_type function requires these to
     already be set up in order to function properly, so cannot be
!    called from there.  */
  
  static tree
! build_atomic_base (tree type)
  {
    tree t;
  
--- 9547,9557 ----
     during initialization of data types to create the 5 basic atomic
     types. The generic build_variant_type function requires these to
     already be set up in order to function properly, so cannot be
!    called from there.  If ALIGN is non-zero, then ensure alignment is
!    overridden to this value.  */
  
  static tree
! build_atomic_base (tree type, unsigned int align)
  {
    tree t;
  
*************** build_atomic_base (tree type)
*** 9561,9566 ****
--- 9562,9570 ----
    t = build_variant_type_copy (type);
    set_type_quals (t, TYPE_QUAL_ATOMIC);
  
+   if (align)
+     TYPE_ALIGN (t) = align;
+ 
    return t;
  }
  
*************** build_common_tree_nodes (bool signed_cha
*** 9648,9660 ****
  
    /* Don't call build_qualified type for atomics.  That routine does
       special processing for atomics, and until they are initialized
!      it's better not to make that call.  */
! 
!   atomicQI_type_node = build_atomic_base (unsigned_intQI_type_node);
!   atomicHI_type_node = build_atomic_base (unsigned_intHI_type_node);
!   atomicSI_type_node = build_atomic_base (unsigned_intSI_type_node);
!   atomicDI_type_node = build_atomic_base (unsigned_intDI_type_node);
!   atomicTI_type_node = build_atomic_base (unsigned_intTI_type_node);
  
    access_public_node = get_identifier ("public");
    access_protected_node = get_identifier ("protected");
--- 9652,9669 ----
  
    /* Don't call build_qualified type for atomics.  That routine does
       special processing for atomics, and until they are initialized
!      it's better not to make that call.
!      
!      Check to see if there is a target override for atomic types.  */
! 
! #define SET_ATOMIC_TYPE_NODE(TYPE, MODE, DEFAULT) 		\
!  (TYPE) = build_atomic_base (DEFAULT, targetm.atomic_align_for_mode (MODE));
! 
!   SET_ATOMIC_TYPE_NODE (atomicQI_type_node, QImode, unsigned_intQI_type_node);
!   SET_ATOMIC_TYPE_NODE (atomicHI_type_node, HImode, unsigned_intHI_type_node);
!   SET_ATOMIC_TYPE_NODE (atomicSI_type_node, SImode, unsigned_intSI_type_node);
!   SET_ATOMIC_TYPE_NODE (atomicDI_type_node, DImode, unsigned_intDI_type_node);
!   SET_ATOMIC_TYPE_NODE (atomicTI_type_node, TImode, unsigned_intTI_type_node);
  
    access_public_node = get_identifier ("public");
    access_protected_node = get_identifier ("protected");