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

Message ID 1300207779-6399-1-git-send-email-justin.ruggles@gmail.com
State Superseded
Headers show

Commit Message

Justin Ruggles March 15, 2011, 4:49 p.m.
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(-)

Comments

Diego Elio Pettenò March 15, 2011, 4:56 p.m. | #1
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.
Mans Rullgard March 15, 2011, 5:05 p.m. | #2
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".
Ronald Bultje March 15, 2011, 5:20 p.m. | #3
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
Justin Ruggles March 15, 2011, 5:25 p.m. | #4
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
Mans Rullgard March 15, 2011, 5:28 p.m. | #5
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.

Patch

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)
+    );
+    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)
+    );
+    return rt;
+}
+
+#endif /* ARCH_X86_32 */
 
 #if HAVE_CMOV
 /* median of 3 */