Patchwork [trans-mem] : Fix PR51830, FAIL: libitm.c/mem(cpy|set)-1.c execution test on x86_32

login
register
mail settings
Submitter Uros Bizjak
Date Jan. 18, 2012, 7:26 p.m.
Message ID <CAFULd4ZX1r8LtYaObruDRB4QN8B0mAMNn15qCN+TdfDi8eJ-nQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/136683/
State New
Headers show

Comments

Uros Bizjak - Jan. 18, 2012, 7:26 p.m.
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.
Patrick Marlier - Jan. 18, 2012, 7:44 p.m.
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.
Uros Bizjak - Jan. 18, 2012, 8:35 p.m.
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.
Richard Henderson - Jan. 23, 2012, 12:29 a.m.
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~

Patch

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)