mathops: convert MULL/MULH/MUL64 to inline functions rather than macros.

diff mbox

This fixes unexpected invalid behavior when used with array values.
It also fixes the fate-acodec-ac3_fixed regression test on x86-32.
---
 libavcodec/x86/mathops.h |   57 +++++++++++++++++++++++++++++++--------------
 1 files changed, 39 insertions(+), 18 deletions(-)

Il giorno mar, 15/03/2011 alle 12.49 -0400, Justin Ruggles ha scritto:
> 
> This fixes unexpected invalid behavior when used with array values.
> It also fixes the fate-acodec-ac3_fixed regression test on x86-32. 

LGTM for the little I can say about x86 asm.

Justin Ruggles <justin.ruggles@gmail.com> writes:

> This fixes unexpected invalid behavior when used with array values.

Array what?  This has nothing to do with arrays.  It's a name collision
plain and simple.

> It also fixes the fate-acodec-ac3_fixed regression test on x86-32.
> ---
>  libavcodec/x86/mathops.h |   57 +++++++++++++++++++++++++++++++--------------
>  1 files changed, 39 insertions(+), 18 deletions(-)
>
>
> diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
> index 5949dfe..4e54886 100644
> --- a/libavcodec/x86/mathops.h
> +++ b/libavcodec/x86/mathops.h
> @@ -26,24 +26,45 @@
>  #include "libavutil/common.h"
>  
>  #if ARCH_X86_32
> -#define MULL(ra, rb, shift) \
> -        ({ int rt, dummy; __asm__ (\
> -            "imull %3               \n\t"\
> -            "shrdl %4, %%edx, %%eax \n\t"\
> -            : "=a"(rt), "=d"(dummy)\
> -            : "a" ((int)(ra)), "rm" ((int)(rb)), "i"(shift));\
> -         rt; })
>  
> -#define MULH(ra, rb) \
> -    ({ int rt, dummy;\
> -     __asm__ ("imull %3\n\t" : "=d"(rt), "=a"(dummy): "a" ((int)(ra)), "rm" ((int)(rb)));\
> -     rt; })
> +#define MULL MULL
> +static av_always_inline av_const int MULL(int a, int b, unsigned shift)
> +{
> +    int rt, dummy;
> +    __asm__ (
> +        "imull %3               \n\t"
> +        "shrdl %4, %%edx, %%eax \n\t"
> +        :"=a"(rt), "=d"(dummy)
> +        :"a"(a), "rm"(b), "i"(shift)
> +    );
> +    return rt;
> +}
>  
> -#define MUL64(ra, rb) \
> -    ({ int64_t rt;\
> -     __asm__ ("imull %2\n\t" : "=A"(rt) : "a" ((int)(ra)), "g" ((int)(rb)));\
> -     rt; })
> -#endif
> +#define MULH MULH
> +static av_always_inline av_const int MULH(int a, int b)
> +{
> +    int rt, dummy;
> +    __asm__ (
> +        "imull %3"
> +        :"=d"(rt), "=a"(dummy)
> +        :"a"(a), "rm"(b)
> +    );

I'd put all that on a single line.

> +    return rt;
> +}
> +
> +#define MUL64 MUL64
> +static av_always_inline av_const int64_t MUL64(int a, int b)
> +{
> +    int64_t rt;
> +    __asm__ (
> +        "imull %2"
> +        :"=A"(rt)
> +        :"a"(a), "g"(b)
> +    );

Ditto.

> +    return rt;
> +}
> +
> +#endif /* ARCH_X86_32 */
>  
>  #if HAVE_CMOV
>  /* median of 3 */

I also wonder why some of those use a "g" constraint while others use
"rm".

Hi,

2011/3/15 Måns Rullgård <mans@mansr.com>:
> Justin Ruggles <justin.ruggles@gmail.com> writes:
>
>> This fixes unexpected invalid behavior when used with array values.
>
> Array what?  This has nothing to do with arrays.  It's a name collision
> plain and simple.
>
>> It also fixes the fate-acodec-ac3_fixed regression test on x86-32.
>> ---
>>  libavcodec/x86/mathops.h |   57 +++++++++++++++++++++++++++++++--------------
>>  1 files changed, 39 insertions(+), 18 deletions(-)
>>
>>
>> diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
>> index 5949dfe..4e54886 100644
>> --- a/libavcodec/x86/mathops.h
>> +++ b/libavcodec/x86/mathops.h
>> @@ -26,24 +26,45 @@
>>  #include "libavutil/common.h"
>>
>>  #if ARCH_X86_32
>> -#define MULL(ra, rb, shift) \
>> -        ({ int rt, dummy; __asm__ (\
>> -            "imull %3               \n\t"\
>> -            "shrdl %4, %%edx, %%eax \n\t"\
>> -            : "=a"(rt), "=d"(dummy)\
>> -            : "a" ((int)(ra)), "rm" ((int)(rb)), "i"(shift));\
>> -         rt; })
>>
>> -#define MULH(ra, rb) \
>> -    ({ int rt, dummy;\
>> -     __asm__ ("imull %3\n\t" : "=d"(rt), "=a"(dummy): "a" ((int)(ra)), "rm" ((int)(rb)));\
>> -     rt; })
>> +#define MULL MULL
>> +static av_always_inline av_const int MULL(int a, int b, unsigned shift)
>> +{
>> +    int rt, dummy;
>> +    __asm__ (
>> +        "imull %3               \n\t"
>> +        "shrdl %4, %%edx, %%eax \n\t"
>> +        :"=a"(rt), "=d"(dummy)
>> +        :"a"(a), "rm"(b), "i"(shift)
>> +    );
>> +    return rt;
>> +}
>>
>> -#define MUL64(ra, rb) \
>> -    ({ int64_t rt;\
>> -     __asm__ ("imull %2\n\t" : "=A"(rt) : "a" ((int)(ra)), "g" ((int)(rb)));\
>> -     rt; })
>> -#endif
>> +#define MULH MULH
>> +static av_always_inline av_const int MULH(int a, int b)
>> +{
>> +    int rt, dummy;
>> +    __asm__ (
>> +        "imull %3"
>> +        :"=d"(rt), "=a"(dummy)
>> +        :"a"(a), "rm"(b)
>> +    );
>
> I'd put all that on a single line.
>
>> +    return rt;
>> +}
>> +
>> +#define MUL64 MUL64
>> +static av_always_inline av_const int64_t MUL64(int a, int b)
>> +{
>> +    int64_t rt;
>> +    __asm__ (
>> +        "imull %2"
>> +        :"=A"(rt)
>> +        :"a"(a), "g"(b)
>> +    );
>
> Ditto.

I personally prefer it the way it is now, at least it makes it
partially readable and I don't have to scramble-search for the colons.

Ronald

Hi,

On 03/15/2011 01:05 PM, Måns Rullgård wrote:

> Justin Ruggles <justin.ruggles@gmail.com> writes:
> 
>> This fixes unexpected invalid behavior when used with array values.
> 
> Array what?  This has nothing to do with arrays.  It's a name collision
> plain and simple.

oh, i completely missed that.. i see now. ac3enc uses the same variable
name.

>> It also fixes the fate-acodec-ac3_fixed regression test on x86-32.
>> ---
>>  libavcodec/x86/mathops.h |   57 +++++++++++++++++++++++++++++++--------------
>>  1 files changed, 39 insertions(+), 18 deletions(-)
>>
>>
>> diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
>> index 5949dfe..4e54886 100644
>> --- a/libavcodec/x86/mathops.h
>> +++ b/libavcodec/x86/mathops.h
>> @@ -26,24 +26,45 @@
>>  #include "libavutil/common.h"
>>  
>>  #if ARCH_X86_32
>> -#define MULL(ra, rb, shift) \
>> -        ({ int rt, dummy; __asm__ (\
>> -            "imull %3               \n\t"\
>> -            "shrdl %4, %%edx, %%eax \n\t"\
>> -            : "=a"(rt), "=d"(dummy)\
>> -            : "a" ((int)(ra)), "rm" ((int)(rb)), "i"(shift));\
>> -         rt; })
>>  
>> -#define MULH(ra, rb) \
>> -    ({ int rt, dummy;\
>> -     __asm__ ("imull %3\n\t" : "=d"(rt), "=a"(dummy): "a" ((int)(ra)), "rm" ((int)(rb)));\
>> -     rt; })
>> +#define MULL MULL
>> +static av_always_inline av_const int MULL(int a, int b, unsigned shift)
>> +{
>> +    int rt, dummy;
>> +    __asm__ (
>> +        "imull %3               \n\t"
>> +        "shrdl %4, %%edx, %%eax \n\t"
>> +        :"=a"(rt), "=d"(dummy)
>> +        :"a"(a), "rm"(b), "i"(shift)
>> +    );
>> +    return rt;
>> +}
>>  
>> -#define MUL64(ra, rb) \
>> -    ({ int64_t rt;\
>> -     __asm__ ("imull %2\n\t" : "=A"(rt) : "a" ((int)(ra)), "g" ((int)(rb)));\
>> -     rt; })
>> -#endif
>> +#define MULH MULH
>> +static av_always_inline av_const int MULH(int a, int b)
>> +{
>> +    int rt, dummy;
>> +    __asm__ (
>> +        "imull %3"
>> +        :"=d"(rt), "=a"(dummy)
>> +        :"a"(a), "rm"(b)
>> +    );
> 
> I'd put all that on a single line.

ok

>> +    return rt;
>> +}
>> +
>> +#define MUL64 MUL64
>> +static av_always_inline av_const int64_t MUL64(int a, int b)
>> +{
>> +    int64_t rt;
>> +    __asm__ (
>> +        "imull %2"
>> +        :"=A"(rt)
>> +        :"a"(a), "g"(b)
>> +    );
> 
> Ditto.

ok

>> +    return rt;
>> +}
>> +
>> +#endif /* ARCH_X86_32 */
>>  
>>  #if HAVE_CMOV
>>  /* median of 3 */
> 
> I also wonder why some of those use a "g" constraint while others use
> "rm".


i don't know much about inline asm, but it appears that it should be
"rm" since imul with 1 arg cannot take an immediate as the argument.

-Justin

Justin Ruggles <justin.ruggles@gmail.com> writes:

> Hi,
>
> On 03/15/2011 01:05 PM, Måns Rullgård wrote:
>
>> Justin Ruggles <justin.ruggles@gmail.com> writes:
>> 
>>> This fixes unexpected invalid behavior when used with array values.
>> 
>> Array what?  This has nothing to do with arrays.  It's a name collision
>> plain and simple.
>
> oh, i completely missed that.. i see now. ac3enc uses the same variable
> name.
>
>>> It also fixes the fate-acodec-ac3_fixed regression test on x86-32.
>>> ---
>>>  libavcodec/x86/mathops.h |   57 +++++++++++++++++++++++++++++++--------------
>>>  1 files changed, 39 insertions(+), 18 deletions(-)
>>>
>>>
>>> diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
>>> index 5949dfe..4e54886 100644
>>> --- a/libavcodec/x86/mathops.h
>>> +++ b/libavcodec/x86/mathops.h
>>> @@ -26,24 +26,45 @@
>>>  #include "libavutil/common.h"
>>>  
>>>  #if ARCH_X86_32
>>> -#define MULL(ra, rb, shift) \
>>> -        ({ int rt, dummy; __asm__ (\
>>> -            "imull %3               \n\t"\
>>> -            "shrdl %4, %%edx, %%eax \n\t"\
>>> -            : "=a"(rt), "=d"(dummy)\
>>> -            : "a" ((int)(ra)), "rm" ((int)(rb)), "i"(shift));\
>>> -         rt; })
>>>  
>>> -#define MULH(ra, rb) \
>>> -    ({ int rt, dummy;\
>>> -     __asm__ ("imull %3\n\t" : "=d"(rt), "=a"(dummy): "a" ((int)(ra)), "rm" ((int)(rb)));\
>>> -     rt; })
>>> +#define MULL MULL
>>> +static av_always_inline av_const int MULL(int a, int b, unsigned shift)
>>> +{
>>> +    int rt, dummy;
>>> +    __asm__ (
>>> +        "imull %3               \n\t"
>>> +        "shrdl %4, %%edx, %%eax \n\t"
>>> +        :"=a"(rt), "=d"(dummy)
>>> +        :"a"(a), "rm"(b), "i"(shift)
>>> +    );
>>> +    return rt;
>>> +}
>>>  
>>> -#define MUL64(ra, rb) \
>>> -    ({ int64_t rt;\
>>> -     __asm__ ("imull %2\n\t" : "=A"(rt) : "a" ((int)(ra)), "g" ((int)(rb)));\
>>> -     rt; })
>>> -#endif
>>> +#define MULH MULH
>>> +static av_always_inline av_const int MULH(int a, int b)
>>> +{
>>> +    int rt, dummy;
>>> +    __asm__ (
>>> +        "imull %3"
>>> +        :"=d"(rt), "=a"(dummy)
>>> +        :"a"(a), "rm"(b)
>>> +    );
>> 
>> I'd put all that on a single line.
>
> ok
>
>>> +    return rt;
>>> +}
>>> +
>>> +#define MUL64 MUL64
>>> +static av_always_inline av_const int64_t MUL64(int a, int b)
>>> +{
>>> +    int64_t rt;
>>> +    __asm__ (
>>> +        "imull %2"
>>> +        :"=A"(rt)
>>> +        :"a"(a), "g"(b)
>>> +    );
>> 
>> Ditto.
>
> ok

Ronald liked it better the way you had it, so do as you prefer.

>>> +    return rt;
>>> +}
>>> +
>>> +#endif /* ARCH_X86_32 */
>>>  
>>>  #if HAVE_CMOV
>>>  /* median of 3 */
>> 
>> I also wonder why some of those use a "g" constraint while others use
>> "rm".
>
> i don't know much about inline asm, but it appears that it should be
> "rm" since imul with 1 arg cannot take an immediate as the argument.

Then change them.