| Message ID | CAFULd4ZX1r8LtYaObruDRB4QN8B0mAMNn15qCN+TdfDi8eJ-nQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 Bizjak<ubizjak@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<ubizjak@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.
On Wed, Jan 18, 2012 at 8:44 PM, Patrick Marlier <patrick.marlier@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.
On 01/19/2012 06:26 AM, Uros Bizjak wrote: > 2012-01-18 Uros Bizjak <ubizjak@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 <ubizjak@gmail.com> > > PR libitm/51830 > * config/x86/sjlj.S (_ITM_beginTransaction) [!__x86_64__]: Load > the first function argument to %eax. Ok. r~
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 leal 4(%esp), %ecx + movl 4(%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)