| Message ID | 528E8D43.2050101@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/21/2013 05:46 PM, Andrew MacLeod wrote: > I'd like to check in this code from the C11 branch so that it is > present in 4.9. > > Its adds a target hook which can be used to override the default > alignment of an atomic type when used with C11's _Atomic qualifier. > There are a couple of ports which have stricter alignment requirements > for an atomic operation than the natural alignment of the integral > type. Today they are just broken with no real facility to repair it. > > If this hook is not utilized by a port, the code is transparent and > should therefore be harmless in the code base. It will enable the > ports that care to experiment with changing alignment and see if their > current problems can be resolved for C11.... and then we can look to > push that into C++11 atomic templates somehow. It will also allow > them to flush out any remaining problems that show up with the > fundamental base code which enables it that went in as part of C11. > > Bootstraps on x86-64 and currently running regression tests. Assuming > everything passes, OK for mainline? > > I wont actually check it in until HP reports back that it actually is > useful to him, I just want to submit it before stage 1 ends :-). > > Andrew H-P reports back that it solves the issues for CRIS : http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02774.html So I'd like to put it in. It would be nice to enable this for the type within the C++11 atomic template too, but Im not sure there is an easy way short of a new __attribute__((atomic)) or something like that... Andrew
On Thu, Nov 21, 2013 at 11:46 PM, Andrew MacLeod <amacleod@redhat.com> wrote: > I'd like to check in this code from the C11 branch so that it is present in > 4.9. > > Its adds a target hook which can be used to override the default alignment > of an atomic type when used with C11's _Atomic qualifier. There are a couple > of ports which have stricter alignment requirements for an atomic operation > than the natural alignment of the integral type. Today they are just > broken with no real facility to repair it. > > If this hook is not utilized by a port, the code is transparent and should > therefore be harmless in the code base. It will enable the ports that care > to experiment with changing alignment and see if their current problems can > be resolved for C11.... and then we can look to push that into C++11 atomic > templates somehow. It will also allow them to flush out any remaining > problems that show up with the fundamental base code which enables it that > went in as part of C11. > > Bootstraps on x86-64 and currently running regression tests. Assuming > everything passes, OK for mainline? > > I wont actually check it in until HP reports back that it actually is useful > to him, I just want to submit it before stage 1 ends :-). Looks good to me, but ! #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); if you have to use a macro please #undef it after the last use. Also if you need to use a macro then make it atomicQI_type_node = BUILD_ATOMIC_TYPE (QI); or even SET_ATOMIC_TYPE_NODE (QI) Thanks, Richard. > Andrew
On Fri, Nov 22, 2013 at 12:21 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Nov 21, 2013 at 11:46 PM, Andrew MacLeod <amacleod@redhat.com> wrote: >> I'd like to check in this code from the C11 branch so that it is present in >> 4.9. >> >> Its adds a target hook which can be used to override the default alignment >> of an atomic type when used with C11's _Atomic qualifier. There are a couple >> of ports which have stricter alignment requirements for an atomic operation >> than the natural alignment of the integral type. Today they are just >> broken with no real facility to repair it. >> >> If this hook is not utilized by a port, the code is transparent and should >> therefore be harmless in the code base. It will enable the ports that care >> to experiment with changing alignment and see if their current problems can >> be resolved for C11.... and then we can look to push that into C++11 atomic >> templates somehow. It will also allow them to flush out any remaining >> problems that show up with the fundamental base code which enables it that >> went in as part of C11. >> >> Bootstraps on x86-64 and currently running regression tests. Assuming >> everything passes, OK for mainline? >> >> I wont actually check it in until HP reports back that it actually is useful >> to him, I just want to submit it before stage 1 ends :-). > > Looks good to me, but > > ! #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); > > if you have to use a macro please #undef it after the last use. > Also if you need to use a macro then make it > > atomicQI_type_node = BUILD_ATOMIC_TYPE (QI); > > or even > > SET_ATOMIC_TYPE_NODE (QI) Btw, I wonder what happens if you declare an array of such over-aligned atomics ... ah, you get an error. typedef int aint __attribute__((aligned(8))); aint a[10]; ./cc1 -quiet t.c t.c:3:1: error: alignment of array elements is greater than element size aint a[10]; ^ good. That won't make arrays of atomics very portable though. Richard. > Thanks, > Richard. > >> Andrew
On Fri, Nov 22, 2013 at 12:21:32PM +0100, Richard Biener wrote: > On Thu, Nov 21, 2013 at 11:46 PM, Andrew MacLeod <amacleod@redhat.com> wrote: > > Its adds a target hook which can be used to override the default alignment > > of an atomic type when used with C11's _Atomic qualifier. There are a couple > > of ports which have stricter alignment requirements for an atomic operation > > than the natural alignment of the integral type. Today they are just > > broken with no real facility to repair it. How do i?86 _Atomic long long and _Atomic long double work btw? At least when they are inside of structures, they have 4 byte alignment. Jakub
On Fri, Nov 22, 2013 at 12:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Nov 22, 2013 at 12:21:32PM +0100, Richard Biener wrote: >> On Thu, Nov 21, 2013 at 11:46 PM, Andrew MacLeod <amacleod@redhat.com> wrote: >> > Its adds a target hook which can be used to override the default alignment >> > of an atomic type when used with C11's _Atomic qualifier. There are a couple >> > of ports which have stricter alignment requirements for an atomic operation >> > than the natural alignment of the integral type. Today they are just >> > broken with no real facility to repair it. > > How do i?86 _Atomic long long and _Atomic long double work btw? > At least when they are inside of structures, they have 4 byte alignment. On powerpc similar funny things happen (for double for example). Richard. > Jakub
On Fri, 22 Nov 2013, Jakub Jelinek wrote: > How do i?86 _Atomic long long and _Atomic long double work btw? > At least when they are inside of structures, they have 4 byte alignment. If the generic libatomic functions are called, they use locking if alignment is inadequate (libatomic presumes natural alignment is the required alignment for size-specific operations). If the operations are inlined or the size-specific libatomic functions are called, you may indeed have problems. Plausible fixes for such issues include: * Probably x86_field_alignment should not reduce alignment for fields of atomic type. * The code in c-common.c resolving size-generic atomic builtins to size-specific ones should not do so if the alignment is less than natural (taking account of possible alignment reductions for fields, so working out pointer target alignment conservatively). (And of course add testcases for any bugs this fixes.)
On 11/22/2013 06:21 AM, Richard Biener wrote: > On Thu, Nov 21, 2013 at 11:46 PM, Andrew MacLeod <amacleod@redhat.com> wrote: >> I'd like to check in this code from the C11 branch so that it is present in >> 4.9. >> >> Its adds a target hook which can be used to override the default alignment >> of an atomic type when used with C11's _Atomic qualifier. There are a couple >> of ports which have stricter alignment requirements for an atomic operation >> than the natural alignment of the integral type. Today they are just >> broken with no real facility to repair it. >> >> If this hook is not utilized by a port, the code is transparent and should >> therefore be harmless in the code base. It will enable the ports that care >> to experiment with changing alignment and see if their current problems can >> be resolved for C11.... and then we can look to push that into C++11 atomic >> templates somehow. It will also allow them to flush out any remaining >> problems that show up with the fundamental base code which enables it that >> went in as part of C11. >> >> Bootstraps on x86-64 and currently running regression tests. Assuming >> everything passes, OK for mainline? >> >> I wont actually check it in until HP reports back that it actually is useful >> to him, I just want to submit it before stage 1 ends :-). > Looks good to me, but > > ! #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); > > if you have to use a macro please #undef it after the last use. > Also if you need to use a macro then make it > > atomicQI_type_node = BUILD_ATOMIC_TYPE (QI); > > or even > > SET_ATOMIC_TYPE_NODE (QI) > > I'll just change it to not use the macro... Andrew
* hooks.h (hook_uint_mode_0): Add Prototype. * hooks.c (hook_uint_mode_0): New default function. * target.def (atomic_align_for_mode): New target hook. * tree.c (build_atomic_base): Add alignment override parameter. (build_common_tree_nodes): Use atomic alignment override. * doc/tm.texi.in (TARGET_ATOMIC_ALIGN_FOR_MODE): Define. * doc/tm.texi (TARGET_ATOMIC_ALIGN_FOR_MODE): Add description. 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: 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: 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"); 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: 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
I'd like to check in this code from the C11 branch so that it is present in 4.9. Its adds a target hook which can be used to override the default alignment of an atomic type when used with C11's _Atomic qualifier. There are a couple of ports which have stricter alignment requirements for an atomic operation than the natural alignment of the integral type. Today they are just broken with no real facility to repair it. If this hook is not utilized by a port, the code is transparent and should therefore be harmless in the code base. It will enable the ports that care to experiment with changing alignment and see if their current problems can be resolved for C11.... and then we can look to push that into C++11 atomic templates somehow. It will also allow them to flush out any remaining problems that show up with the fundamental base code which enables it that went in as part of C11. Bootstraps on x86-64 and currently running regression tests. Assuming everything passes, OK for mainline? I wont actually check it in until HP reports back that it actually is useful to him, I just want to submit it before stage 1 ends :-). Andrew