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

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

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

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.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 1269 bytes --]

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)

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

[Patrick Marlier]

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.

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

[Uros Bizjak]

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.

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

[Patrick Marlier]

On 01/18/2012 03:35 PM, Uros Bizjak wrote:
> Please note that all recent x86 processors implement store forwarding,
> so passing arguments through memory is mostly a non-issue nowadays.

Ok. Thanks :)

> 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;

Thanks. :)
--
Patrick.

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

[Uros Bizjak]

On Wed, Jan 18, 2012 at 10:16 PM, Patrick Marlier
<patrick.marlier@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.

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

[Torvald Riegel]

On Wed, 2012-01-18 at 22:25 +0100, Uros Bizjak wrote:
> On Wed, Jan 18, 2012 at 10:16 PM, Patrick Marlier
> <patrick.marlier@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

[Torvald Riegel]

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

[Uros Bizjak]

On Thu, Jan 19, 2012 at 1:24 PM, Torvald Riegel <triegel@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.

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

[Richard Henderson]

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~

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

[Richard Henderson]

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~