Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On 01/19/2012 06:26 AM, Uros Bizjak wrote: 2012-01-18 Uros Bizjak ubiz...@gmail.com PR libitm/51830 * builtin-types.def (BT_FN_UINT_UINT_VAR): New. * gtm-builtins.def (BUILT_IN_TM_START): Declare as BT_FN_UINT_UINT_VAR. libitm/ChangeLog: 2012-01-18 Uros Bizjak ubiz...@gmail.com PR libitm/51830 * config/x86/sjlj.S (_ITM_beginTransaction) [!__x86_64__]: Load the first function argument to %eax. Ok. r~
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On 01/20/2012 12:51 AM, Uros Bizjak wrote: OTOH, in GTM_beginTransaction we can still access other variable arguments through the pointer to CFA. Well, no, not really. If we really want GTM_beginTransaction to have access to the variadic portions, we'll need to have the sjlj stub pass in a va_list. Thankfully we can generally ignore this until we actually need those extra bits. Which is not in the near-term cards. r~
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On Wed, 2012-01-18 at 22:25 +0100, Uros Bizjak wrote: On Wed, Jan 18, 2012 at 10:16 PM, Patrick Marlier patrick.marl...@gmail.com wrote: IMO, whatever the future decision would be, we shouldn't leave one part of the compiler out-of-sync from the other. Proposed patch fixes _current_ situation, where in the future, it is expected that compiler and library changes in sync... So in order to keep them in sync, this should be also applied to libitm if your solution is chosen (At least to avoid confusion). If the Intel TM-ABI (no idea what's the status of this specification) specifies variadic function and regparm, it should be changed too. Index: libitm.h === --- libitm.h(revision 183273) +++ libitm.h(working copy) @@ -136,7 +136,7 @@ typedef uint64_t _ITM_transactionId_t; /* Transact extern _ITM_transactionId_t _ITM_getTransactionId(void) ITM_REGPARM; -extern uint32_t _ITM_beginTransaction(uint32_t, ...) ITM_REGPARM; +extern uint32_t _ITM_beginTransaction(uint32_t, ...); extern void _ITM_abortTransaction(_ITM_abortReason) ITM_REGPARM ITM_NORETURN; The spec does say that all function should be regparm(2), but I agree that the above is less confusing. The attribute is ignored, but perhaps a comment would clear this confusion even more. Uros, thanks for spotting the vararg issue. This looks okay to me, but Richard Henderson will have to OK this. If regparm(2) cannot work with variadic functions on x86, then I'd prefer removing the regparm. beginTransaction was switched to being variadic to allow communicating which kinds of versions a compiler has generated for the transaction's code (besides the default instrumentation that we have right now). I'd believe Ulrich Drepper's experience that making this variadic is better than restricting this to 64b (minus 10 bits or so already in use). BTW, would regparm(2) optimize on any arch/platform besides 32b x86? What about x32? Note that if we remove the regparm, we should also remove it on the other functions associated with txn begin (GTM_beginTransaction etc.). Torvald
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On Thu, 2012-01-19 at 13:24 +0100, Torvald Riegel wrote: Note that if we remove the regparm, we should also remove it on the other functions associated with txn begin (GTM_beginTransaction etc.). And update libitm.texi ...
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On Thu, Jan 19, 2012 at 1:24 PM, Torvald Riegel trie...@redhat.com wrote: The spec does say that all function should be regparm(2), but I agree that the above is less confusing. The attribute is ignored, but perhaps a comment would clear this confusion even more. Uros, thanks for spotting the vararg issue. This looks okay to me, but Richard Henderson will have to OK this. If regparm(2) cannot work with variadic functions on x86, then I'd prefer removing the regparm. beginTransaction was switched to being variadic to allow communicating which kinds of versions a compiler has generated for the transaction's code (besides the default instrumentation that we have right now). I'd believe Ulrich Drepper's experience that making this variadic is better than restricting this to 64b (minus 10 bits or so already in use). BTW, would regparm(2) optimize on any arch/platform besides 32b x86? What about x32? No, regparm is effective only on x86_32. x32 strictly follows x86_64 ABI. Note that if we remove the regparm, we should also remove it on the other functions associated with txn begin (GTM_beginTransaction etc.). No, this is not needed. The patch adds the move that loads %eax with the first parameter from function arguments and pass it via regparm ABI to GTM_beginTransaction. OTOH, in GTM_beginTransaction we can still access other variable arguments through the pointer to CFA. Uros.
[PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
Hello! Attached three-liner patch fixes the declaration of BUILT_IN_TM_START (AKA _ITM_beginTransaction) to match its declaration from the libitm.h ABI. This mismatch was the core problem for FAILed libitm.c/mem(cpy|set)-1.c execution tests on x86_32. Following that change, we need to teach _ITM_beginTransaction where to find its first argument, so it can be passed to GTM_begin_transaction. There was some discussion on where to pass arguments to regparm decorated vararg functions. Well, as the ABI is pretty clear - regparm should be ignored in this case, so all function arguments have to be passed in memory, even if that means that the value is kicked to the memory before the call, and pulled back into the register in _ITM_beginTransaction. 2012-01-18 Uros Bizjak ubiz...@gmail.com PR libitm/51830 * builtin-types.def (BT_FN_UINT_UINT_VAR): New. * gtm-builtins.def (BUILT_IN_TM_START): Declare as BT_FN_UINT_UINT_VAR. libitm/ChangeLog: 2012-01-18 Uros Bizjak ubiz...@gmail.com PR libitm/51830 * config/x86/sjlj.S (_ITM_beginTransaction) [!__x86_64__]: Load the first function argument to %eax. The patch touches generic libitm part, so I have tested it on i686-pc-linux-gnu (where it fixes all failures), x86_64-pc-linux-gnu and alphaev68-pc-linux-gnu. OK for mainline? Uros. Index: libitm/config/x86/sjlj.S === --- libitm/config/x86/sjlj.S(revision 183277) +++ libitm/config/x86/sjlj.S(working copy) @@ -79,6 +79,7 @@ ret #else leal4(%esp), %ecx + movl4(%esp), %eax subl$28, %esp cfi_def_cfa_offset(32) movl%ecx, 8(%esp) Index: gcc/builtin-types.def === --- gcc/builtin-types.def (revision 183277) +++ gcc/builtin-types.def (working copy) @@ -498,6 +498,8 @@ BT_VOID, BT_CONST_PTR) DEF_FUNCTION_TYPE_VAR_1 (BT_FN_INT_CONST_STRING_VAR, BT_INT, BT_CONST_STRING) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_UINT_UINT_VAR, +BT_UINT, BT_UINT) DEF_FUNCTION_TYPE_VAR_2 (BT_FN_INT_FILEPTR_CONST_STRING_VAR, BT_INT, BT_FILEPTR, BT_CONST_STRING) Index: gcc/gtm-builtins.def === --- gcc/gtm-builtins.def(revision 183277) +++ gcc/gtm-builtins.def(working copy) @@ -1,5 +1,5 @@ DEF_TM_BUILTIN (BUILT_IN_TM_START, _ITM_beginTransaction, - BT_FN_UINT_UINT, ATTR_TM_NOTHROW_RT_LIST) + BT_FN_UINT_UINT_VAR, ATTR_TM_NOTHROW_RT_LIST) DEF_TM_BUILTIN (BUILT_IN_TM_COMMIT, _ITM_commitTransaction, BT_FN_VOID, ATTR_TM_NOTHROW_LIST)
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On 01/18/2012 02:26 PM, Uros Bizjak wrote: Hello! Attached three-liner patch fixes the declaration of BUILT_IN_TM_START (AKA _ITM_beginTransaction) to match its declaration from the libitm.h ABI. This mismatch was the core problem for FAILed libitm.c/mem(cpy|set)-1.c execution tests on x86_32. Following that change, we need to teach _ITM_beginTransaction where to find its first argument, so it can be passed to GTM_begin_transaction. There was some discussion on where to pass arguments to regparm decorated vararg functions. Well, as the ABI is pretty clear - regparm should be ignored in this case, so all function arguments have to be passed in memory, even if that means that the value is kicked to the memory before the call, and pulled back into the register in _ITM_beginTransaction. 2012-01-18 Uros Bizjakubiz...@gmail.com PR libitm/51830 * builtin-types.def (BT_FN_UINT_UINT_VAR): New. * gtm-builtins.def (BUILT_IN_TM_START): Declare as BT_FN_UINT_UINT_VAR. libitm/ChangeLog: 2012-01-18 Uros Bizjakubiz...@gmail.com PR libitm/51830 * config/x86/sjlj.S (_ITM_beginTransaction) [!__x86_64__]: Load the first function argument to %eax. The patch touches generic libitm part, so I have tested it on i686-pc-linux-gnu (where it fixes all failures), x86_64-pc-linux-gnu and alphaev68-pc-linux-gnu. OK for mainline? Uros. My main concern here is performance... Indeed, in case of libitm using Hardware Transactional Memory, it could be great to use registers for parameters. I would prefer to remove the variadic function as proposed here: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg01784.html As Torvald wrote, it was in case for hypothetical future parameters. So I would agree to do: extern uint32_t _ITM_beginTransaction(uint32_t,uint32_t) ITM_REGPARM; At least, it provides a new parameter for future use and do not use the stack for parameters. Other thoughts? Thanks. -- Patrick Marlier.
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On Wed, Jan 18, 2012 at 8:44 PM, Patrick Marlier patrick.marl...@gmail.com wrote: There was some discussion on where to pass arguments to regparm decorated vararg functions. Well, as the ABI is pretty clear - regparm should be ignored in this case, so all function arguments have to be passed in memory, even if that means that the value is kicked to the memory before the call, and pulled back into the register in _ITM_beginTransaction. My main concern here is performance... Indeed, in case of libitm using Hardware Transactional Memory, it could be great to use registers for parameters. I would prefer to remove the variadic function as proposed here: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg01784.html Please note that all recent x86 processors implement store forwarding, so passing arguments through memory is mostly a non-issue nowadays. As Torvald wrote, it was in case for hypothetical future parameters. So I would agree to do: extern uint32_t _ITM_beginTransaction(uint32_t,uint32_t) ITM_REGPARM; At least, it provides a new parameter for future use and do not use the stack for parameters. Other thoughts? IMO, whatever the future decision would be, we shouldn't leave one part of the compiler out-of-sync from the other. Proposed patch fixes _current_ situation, where in the future, it is expected that compiler and library changes in sync... Uros.
Re: [PATCH, trans-mem]: Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32
On Wed, Jan 18, 2012 at 10:16 PM, Patrick Marlier patrick.marl...@gmail.com wrote: IMO, whatever the future decision would be, we shouldn't leave one part of the compiler out-of-sync from the other. Proposed patch fixes _current_ situation, where in the future, it is expected that compiler and library changes in sync... So in order to keep them in sync, this should be also applied to libitm if your solution is chosen (At least to avoid confusion). If the Intel TM-ABI (no idea what's the status of this specification) specifies variadic function and regparm, it should be changed too. Index: libitm.h === --- libitm.h (revision 183273) +++ libitm.h (working copy) @@ -136,7 +136,7 @@ typedef uint64_t _ITM_transactionId_t; /* Transact extern _ITM_transactionId_t _ITM_getTransactionId(void) ITM_REGPARM; -extern uint32_t _ITM_beginTransaction(uint32_t, ...) ITM_REGPARM; +extern uint32_t _ITM_beginTransaction(uint32_t, ...); extern void _ITM_abortTransaction(_ITM_abortReason) ITM_REGPARM ITM_NORETURN; The spec does say that all function should be regparm(2), but I agree that the above is less confusing. The attribute is ignored, but perhaps a comment would clear this confusion even more. Uros.
