Message ID | 5458FE9C.2090409@redhat.com |
---|---|
State | New |
Headers | show |
On 11/04/2014 05:28 PM, Andrew MacLeod wrote: > + bool > + default_can_compare_and_swap_p (machine_mode mode, bool allow_libcall) > + { > + return can_compare_and_swap_p (mode, allow_libcall); > + } This is silly. I think the problem you point out can be better fixed by moving the can_compare_and_swap_p prototype elsewhere. r~
On 11/04/2014 12:25 PM, Richard Henderson wrote: > On 11/04/2014 05:28 PM, Andrew MacLeod wrote: >> + bool >> + default_can_compare_and_swap_p (machine_mode mode, bool allow_libcall) >> + { >> + return can_compare_and_swap_p (mode, allow_libcall); >> + } > This is silly. I think the problem you point out can be better fixed by moving > the can_compare_and_swap_p prototype elsewhere. > yeah, except it uses some of the optab table stuff that is static to optabs.c... so the basic functionality remains there. we could stick a wrapper somewhere else... but thats effectively what this does. we are asking a question about target behaviour... Im open to other suggestions.. Andrew
On 11/04/2014 06:56 PM, Andrew MacLeod wrote: > On 11/04/2014 12:25 PM, Richard Henderson wrote: >> On 11/04/2014 05:28 PM, Andrew MacLeod wrote: >>> + bool >>> + default_can_compare_and_swap_p (machine_mode mode, bool allow_libcall) >>> + { >>> + return can_compare_and_swap_p (mode, allow_libcall); >>> + } >> This is silly. I think the problem you point out can be better fixed by moving >> the can_compare_and_swap_p prototype elsewhere. >> > yeah, except it uses some of the optab table stuff that is static to > optabs.c... so the basic functionality remains there. I said move the prototype. Of course the implementation remains where it is. r~
On 11/04/2014 12:57 PM, Richard Henderson wrote: > On 11/04/2014 06:56 PM, Andrew MacLeod wrote: >> On 11/04/2014 12:25 PM, Richard Henderson wrote: >>> On 11/04/2014 05:28 PM, Andrew MacLeod wrote: >>>> + bool >>>> + default_can_compare_and_swap_p (machine_mode mode, bool allow_libcall) >>>> + { >>>> + return can_compare_and_swap_p (mode, allow_libcall); >>>> + } >>> This is silly. I think the problem you point out can be better fixed by moving >>> the can_compare_and_swap_p prototype elsewhere. >>> >> yeah, except it uses some of the optab table stuff that is static to >> optabs.c... so the basic functionality remains there. > I said move the prototype. Of course the implementation remains where it is. > prototype is in optabs.h where it belongs since its defined in optabs.c. :-) I'm not sure why this is much different than something like the targhook for builtin_support_vector_misalignment(), other than we are calling the routine in optabs.c rather than putting the actual code in targhooks.c. from targhooks.c: bool default_builtin_support_vector_misalignment (machine_mode mode, const_tree type, <...>) { if (optab_handler (movmisalign_optab, mode) != CODE_FOR_nothing) return true; return false; } the idea is to move all the functionality that front ends need into well defined and controlled places so we can increase the separation. "can perform a compare_and_swap operation" is clearly a target specific question isn't it? Andrew
On November 4, 2014 7:30:18 PM CET, Andrew MacLeod <amacleod@redhat.com> wrote: >On 11/04/2014 12:57 PM, Richard Henderson wrote: >> On 11/04/2014 06:56 PM, Andrew MacLeod wrote: >>> On 11/04/2014 12:25 PM, Richard Henderson wrote: >>>> On 11/04/2014 05:28 PM, Andrew MacLeod wrote: >>>>> + bool >>>>> + default_can_compare_and_swap_p (machine_mode mode, bool >allow_libcall) >>>>> + { >>>>> + return can_compare_and_swap_p (mode, allow_libcall); >>>>> + } >>>> This is silly. I think the problem you point out can be better >fixed by moving >>>> the can_compare_and_swap_p prototype elsewhere. >>>> >>> yeah, except it uses some of the optab table stuff that is static to >>> optabs.c... so the basic functionality remains there. >> I said move the prototype. Of course the implementation remains >where it is. >> >prototype is in optabs.h where it belongs since its defined in >optabs.c. :-) > >I'm not sure why this is much different than something like the >targhook >for builtin_support_vector_misalignment(), other than we are calling >the >routine in optabs.c rather than putting the actual code in targhooks.c. > >from targhooks.c: >bool >default_builtin_support_vector_misalignment (machine_mode mode, >const_tree type, <...>) > { > if (optab_handler (movmisalign_optab, mode) != CODE_FOR_nothing) > return true; > return false; >} > >the idea is to move all the functionality that front ends need into >well >defined and controlled places so we can increase the separation. "can >perform a compare_and_swap operation" is clearly a target specific >question isn't it? I would rather question what is so special about java that it needs to ask that and other frontends not. Don't we have generic atomics support now? Richard. >Andrew
On 11/04/2014 02:53 PM, Richard Biener wrote: > On November 4, 2014 7:30:18 PM CET, Andrew MacLeod <amacleod@redhat.com> wrote: >> On 11/04/2014 12:57 PM, Richard Henderson wrote: >>> On 11/04/2014 06:56 PM, Andrew MacLeod wrote: >>>> On 11/04/2014 12:25 PM, Richard Henderson wrote: >>>>> On 11/04/2014 05:28 PM, Andrew MacLeod wrote: >>>>>> + bool >>>>>> + default_can_compare_and_swap_p (machine_mode mode, bool >> allow_libcall) >>>>>> + { >>>>>> + return can_compare_and_swap_p (mode, allow_libcall); >>>>>> + } >>>>> This is silly. I think the problem you point out can be better >> fixed by moving >>>>> the can_compare_and_swap_p prototype elsewhere. >>>>> >>>> yeah, except it uses some of the optab table stuff that is static to >>>> optabs.c... so the basic functionality remains there. >>> I said move the prototype. Of course the implementation remains >> where it is. >> prototype is in optabs.h where it belongs since its defined in >> optabs.c. :-) >> >> I'm not sure why this is much different than something like the >> targhook >> for builtin_support_vector_misalignment(), other than we are calling >> the >> routine in optabs.c rather than putting the actual code in targhooks.c. >> > >from targhooks.c: >> bool >> default_builtin_support_vector_misalignment (machine_mode mode, >> const_tree type, <...>) >> { >> if (optab_handler (movmisalign_optab, mode) != CODE_FOR_nothing) >> return true; >> return false; >> } >> >> the idea is to move all the functionality that front ends need into >> well >> defined and controlled places so we can increase the separation. "can >> perform a compare_and_swap operation" is clearly a target specific >> question isn't it? > I would rather question what is so special about java that it needs to ask that and other frontends not. Don't we have generic atomics support now? > > Richard. > True... I don't know if this is a thing that simply predates our current level of support or if it is something else that is java specific for its builtins. Don't know enough about java to comment. aph? Looks like you wrote the originals in 2006... Can the java CAS builtins simply use our current atomic calls rather than doing their own thing and querying whether the target has a sync compare and swap operation? Andrew
On 11/04/2014 03:13 PM, Andrew MacLeod wrote: > >from targhooks.c: >>> bool >>> default_builtin_support_vector_misalignment (machine_mode mode, >>> const_tree type, <...>) >>> { >>> if (optab_handler (movmisalign_optab, mode) != CODE_FOR_nothing) >>> return true; >>> return false; >>> } >>> >>> the idea is to move all the functionality that front ends need into >>> well >>> defined and controlled places so we can increase the separation. "can >>> perform a compare_and_swap operation" is clearly a target specific >>> question isn't it? >> I would rather question what is so special about java that it needs >> to ask that and other frontends not. Don't we have generic atomics >> support now? >> >> Richard. >> > True... I don't know if this is a thing that simply predates our > current level of support or if it is something else that is java > specific for its builtins. > Don't know enough about java to comment. > > aph? Looks like you wrote the originals in 2006... Can the java CAS > builtins simply use our current atomic calls rather than doing their > own thing and querying whether the target has a sync compare and swap > operation? > > Andrew It looks like java is deciding whether or not GCC can inline atomic operations or not, and if it can't, doesn't want the atomic operations... which presumably means there is no dependency on libatomic at runtime. A call to can_compare_and_swap_p(mode) is analogous to a compile time version of folding atomic_always_lock_free(mode) to a constant... Frankly that seems like a reasonable question for some front end to ask... and elect not to emit atomic calls if so desired. (which is what java is doing I think) whether it still needs to do that is a question for some java person. Andrew
On 11/06/2014 05:57 PM, Andrew MacLeod wrote: > It looks like java is deciding whether or not GCC can inline atomic > operations or not, and if it can't, doesn't want the atomic > operations... which presumably means there is no dependency on > libatomic at runtime. > > A call to can_compare_and_swap_p(mode) is analogous to a compile time > version of folding atomic_always_lock_free(mode) to a constant... > Frankly that seems like a reasonable question for some front end to > ask... and elect not to emit atomic calls if so desired. (which is what > java is doing I think) > > whether it still needs to do that is a question for some java person. I did it because some targets did not have library support for some builtins, so a compile would fail with a (to a Java programmer) baffling error message. The Java operations certainly should use the generic builtins. Andrew.
On 11/06/2014 01:23 PM, Andrew Haley wrote: > On 11/06/2014 05:57 PM, Andrew MacLeod wrote: >> It looks like java is deciding whether or not GCC can inline atomic >> operations or not, and if it can't, doesn't want the atomic >> operations... which presumably means there is no dependency on >> libatomic at runtime. >> >> A call to can_compare_and_swap_p(mode) is analogous to a compile time >> version of folding atomic_always_lock_free(mode) to a constant... >> Frankly that seems like a reasonable question for some front end to >> ask... and elect not to emit atomic calls if so desired. (which is what >> java is doing I think) >> >> whether it still needs to do that is a question for some java person. > I did it because some targets did not have library support for some > builtins, so a compile would fail with a (to a Java programmer) > baffling error message. > > The Java operations certainly should use the generic builtins. > > Thanks Andrew 1) Given that the compiler *always* provides support via libatomic now (even if it is via locks), does that mean that VMSupportsCS8_builtin() should always return true? or should we map to that a call to __atomic_always_lock_free() ? (that always gets folded to a true or false at compile time) my guess is the latter? 2) and in compareAndSwapLong_builtin(), thre is a wonky bit: /* We don't trust flag_use_atomic_builtins for multi-word compareAndSwap. Some machines such as ARM have atomic libfuncs but not the multi-word versions. */ if (can_compare_and_swap_p (mode, (flag_use_atomic_builtins && GET_MODE_SIZE (mode) <= UNITS_PER_WORD))) <..> /* generate 8 byte CAS */ I gather we dont need to do anything special here anymore either? As an observation of inconsistency, compareAndSwapObject_builtin doesn't do that check before calling the 8 byte CAS : machine_mode mode = TYPE_MODE (ptr_type_node); if (can_compare_and_swap_p (mode, flag_use_atomic_builtins)) { tree addr, stmt; enum built_in_function builtin; UNMARSHAL5 (orig_call); builtin = (POINTER_SIZE == 32 ? BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_4 : BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_8); addr = build_addr_sum (value_type, obj_arg, offset_arg); 3) And finally, is flag_use_atomic_builtins suppose to turn them off completely? Right now it is passed in to the second parameter of can_compare_and_swap_p, which really just says can we compare and swap without calling a libfunc.. so currently if the flag is 0, but there is native support, the call is generated anyway. should that condition really be: if (flag_use_atomic_builtins) { <...> /* generate atomic call */ } Thanks Andrew.
On 06/11/14 19:05, Andrew MacLeod wrote: > > > 1) Given that the compiler *always* provides support via libatomic now > (even if it is via locks), does that mean that VMSupportsCS8_builtin() > should always return true? > > or should we map to that a call to __atomic_always_lock_free() ? (that > always gets folded to a true or false at compile time) my guess is the > latter? Perhaps so. The problem is that some targets can't do CAS on 64-bit doublewords. > 2) and in compareAndSwapLong_builtin(), thre is a wonky bit: > > /* We don't trust flag_use_atomic_builtins for multi-word compareAndSwap. > Some machines such as ARM have atomic libfuncs but not the multi-word > versions. */ > if (can_compare_and_swap_p (mode, > (flag_use_atomic_builtins > && GET_MODE_SIZE (mode) <= UNITS_PER_WORD))) > <..> /* generate 8 byte CAS */ > > I gather we dont need to do anything special here anymore either? As > an observation of inconsistency, > compareAndSwapObject_builtin doesn't do that check before calling the 8 > byte CAS : I believe that any machine which has 64-bit pointers and can do CAS can do a 64-bit CAS. I'm worried about 32-bit machines trying to do a 64-bit CAS. > 3) And finally, is flag_use_atomic_builtins suppose to turn them off > completely? Right now it is passed in to the second parameter of > can_compare_and_swap_p, which really just says can we compare and swap > without calling a libfunc.. so currently if the flag is 0, but there > is native support, the call is generated anyway. should that condition > really be: > > if (flag_use_atomic_builtins) > { > <...> /* generate atomic call */ > } I'm sorry, I really can't remember. I can't think of any reason to want to turn off builtin support. You have to remember that all this was written when our support for atomic builtins was seriously flaky and we would just punt back to the user anything we hadn't written yet. Andrew.
On 11/07/2014 04:31 AM, Andrew Haley wrote: > On 06/11/14 19:05, Andrew MacLeod wrote: >> >> 1) Given that the compiler *always* provides support via libatomic now >> (even if it is via locks), does that mean that VMSupportsCS8_builtin() >> should always return true? >> >> or should we map to that a call to __atomic_always_lock_free() ? (that >> always gets folded to a true or false at compile time) my guess is the >> latter? > Perhaps so. The problem is that some targets can't do CAS on 64-bit > doublewords. with libatomic present, I believe they always can, even if it drops to a lock implementation. > I'm sorry, I really can't remember. I can't think of any reason to > want to turn off builtin support. You have to remember that all this > was written when our support for atomic builtins was seriously flaky > and we would just punt back to the user anything we hadn't written > yet. > > No worries, i cant remember why i did something last year, let along 8 years ago :-) I'll take a best stab and we'll see what happens :-) Andrew
* target.def (can_compare_and_swap_p): Define target hook. * targhooks.c (default_can_compare_and_swap_p): Provide default hook. * targhooks.h (default_can_compare_and_swap_p): Provide prototype. * doc/tm.texi (target_can_compare_and_swap_p): Provide description. * doc/tm.texi.in (TARGET_CAN_COMPARE_AND_SWAP_P): Provide location. * java/builtins.c. Remove backend includes, include target.h. (compareAndSwapInt_builtin, compareAndSwapLong_builtin, compareAndSwapObject_builtin, VMSupportsCS8_builtin): Use compare_and_swap_p target hook. Index: target.def =================================================================== *** target.def (revision 217057) --- target.def (working copy) *************** specific target options and the caller d *** 5185,5190 **** --- 5185,5199 ---- bool, (tree caller, tree callee), default_target_can_inline_p) + DEFHOOK + (can_compare_and_swap_p, + "This macro returns true if the target can perform an atomic\n\ + compare_and_swap operation on an object of machine mode @var{mode}.\n\ + @var{allow_libcall} is a boolean which is used to indicate whether the\n\ + operation is allowed to be performed with a libcall.", + bool, (machine_mode mode, bool allow_libcall), + default_can_compare_and_swap_p) + HOOK_VECTOR_END (target_option) /* For targets that need to mark extra registers as live on entry to Index: targhooks.c =================================================================== *** targhooks.c (revision 217057) --- targhooks.c (working copy) *************** default_target_can_inline_p (tree caller *** 1322,1327 **** --- 1322,1334 ---- return ret; } + bool + default_can_compare_and_swap_p (machine_mode mode, bool allow_libcall) + { + return can_compare_and_swap_p (mode, allow_libcall); + } + + #ifndef HAVE_casesi # define HAVE_casesi 0 #endif Index: targhooks.h =================================================================== *** targhooks.h (revision 217057) --- targhooks.h (working copy) *************** extern tree std_gimplify_va_arg_expr (tr *** 220,224 **** --- 220,226 ---- extern bool can_use_doloop_if_innermost (const widest_int &, const widest_int &, unsigned int, bool); + extern bool default_can_compare_and_swap_p (machine_mode, bool); + #endif /* GCC_TARGHOOKS_H */ Index: doc/tm.texi =================================================================== *** doc/tm.texi (revision 217057) --- doc/tm.texi (working copy) *************** default, inlining is not allowed if the *** 9742,9747 **** --- 9742,9754 ---- specific target options and the caller does not use the same options. @end deftypefn + @deftypefn {Target Hook} bool TARGET_CAN_COMPARE_AND_SWAP_P (machine_mode @var{mode}, bool @var{allow_libcall}) + This macro returns true if the target can perform an atomic + compare_and_swap operation on an object of machine mode @var{mode}. + @var{allow_libcall} is a boolean which is used to indicate whether the + operation is allowed to be performed with a libcall. + @end deftypefn + @node Emulated TLS @section Emulating TLS @cindex Emulated TLS Index: doc/tm.texi.in =================================================================== *** doc/tm.texi.in (revision 217057) --- doc/tm.texi.in (working copy) *************** on this implementation detail. *** 7227,7232 **** --- 7227,7234 ---- @hook TARGET_CAN_INLINE_P + @hook TARGET_CAN_COMPARE_AND_SWAP_P + @node Emulated TLS @section Emulating TLS @cindex Emulated TLS Index: java/builtins.c =================================================================== *** java/builtins.c (revision 217057) --- java/builtins.c (working copy) *************** The Free Software Foundation is independ *** 37,49 **** #include "flags.h" #include "langhooks.h" #include "java-tree.h" ! ! /* FIXME: All these headers are necessary for sync_compare_and_swap. ! Front ends should never have to look at that. */ ! #include "rtl.h" ! #include "insn-codes.h" ! #include "expr.h" ! #include "optabs.h" static tree max_builtin (tree, tree); static tree min_builtin (tree, tree); --- 37,43 ---- #include "flags.h" #include "langhooks.h" #include "java-tree.h" ! #include "target.h" static tree max_builtin (tree, tree); static tree min_builtin (tree, tree); *************** compareAndSwapInt_builtin (tree method_r *** 320,326 **** tree orig_call) { machine_mode mode = TYPE_MODE (int_type_node); ! if (can_compare_and_swap_p (mode, flag_use_atomic_builtins)) { tree addr, stmt; enum built_in_function fncode = BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_4; --- 314,321 ---- tree orig_call) { machine_mode mode = TYPE_MODE (int_type_node); ! if (targetm.target_option.can_compare_and_swap_p (mode, ! flag_use_atomic_builtins)) { tree addr, stmt; enum built_in_function fncode = BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_4; *************** compareAndSwapLong_builtin (tree method_ *** 344,352 **** /* We don't trust flag_use_atomic_builtins for multi-word compareAndSwap. Some machines such as ARM have atomic libfuncs but not the multi-word versions. */ ! if (can_compare_and_swap_p (mode, ! (flag_use_atomic_builtins ! && GET_MODE_SIZE (mode) <= UNITS_PER_WORD))) { tree addr, stmt; enum built_in_function fncode = BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_8; --- 339,347 ---- /* We don't trust flag_use_atomic_builtins for multi-word compareAndSwap. Some machines such as ARM have atomic libfuncs but not the multi-word versions. */ ! if (targetm.target_option.can_compare_and_swap_p (mode, ! (flag_use_atomic_builtins ! && GET_MODE_SIZE (mode) <= UNITS_PER_WORD))) { tree addr, stmt; enum built_in_function fncode = BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_8; *************** compareAndSwapObject_builtin (tree metho *** 366,372 **** tree orig_call) { machine_mode mode = TYPE_MODE (ptr_type_node); ! if (can_compare_and_swap_p (mode, flag_use_atomic_builtins)) { tree addr, stmt; enum built_in_function builtin; --- 361,368 ---- tree orig_call) { machine_mode mode = TYPE_MODE (ptr_type_node); ! if (targetm.target_option.can_compare_and_swap_p (mode, ! flag_use_atomic_builtins)) { tree addr, stmt; enum built_in_function builtin; *************** VMSupportsCS8_builtin (tree method_retur *** 446,452 **** { machine_mode mode = TYPE_MODE (long_type_node); gcc_assert (method_return_type == boolean_type_node); ! if (can_compare_and_swap_p (mode, false)) return boolean_true_node; else return boolean_false_node; --- 442,448 ---- { machine_mode mode = TYPE_MODE (long_type_node); gcc_assert (method_return_type == boolean_type_node); ! if (targetm.target_option.can_compare_and_swap_p (mode, false)) return boolean_true_node; else return boolean_false_node;