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

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.


Thanks. :)
--
Patrick.

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.

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

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

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.

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~