[FFmpeg-devel] avcodec/x86/mathops: clip constants used with shift instructions within inline assembly

From: Rémi Denis-Courmont <remi@remlab.net>

Fixes assembling with binutil as >= 2.41

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/x86/mathops.h | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

James Almer:
> From: Rémi Denis-Courmont <remi@remlab.net>
> 
> Fixes assembling with binutil as >= 2.41
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/x86/mathops.h | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
> index 6298f5ed19..ca7e2dffc1 100644
> --- a/libavcodec/x86/mathops.h
> +++ b/libavcodec/x86/mathops.h
> @@ -35,12 +35,20 @@
>  static av_always_inline av_const int MULL(int a, int b, unsigned shift)
>  {
>      int rt, dummy;
> +    if (__builtin_constant_p(shift))

We actually have av_builtin_constant_p. Is it guaranteed that all
compilers supporting inline ASM also support __builtin_constant_p?

>      __asm__ (
>          "imull %3               \n\t"
>          "shrdl %4, %%edx, %%eax \n\t"
>          :"=a"(rt), "=d"(dummy)
> -        :"a"(a), "rm"(b), "ci"((uint8_t)shift)
> +        :"a"(a), "rm"(b), "i"(shift & 0x1F)
>      );
> +    else
> +        __asm__ (
> +            "imull %3               \n\t"
> +            "shrdl %4, %%edx, %%eax \n\t"
> +            :"=a"(rt), "=d"(dummy)
> +            :"a"(a), "rm"(b), "c"((uint8_t)shift)
> +        );
>      return rt;
>  }
>  
> @@ -113,19 +121,31 @@ __asm__ volatile(\
>  // avoid +32 for shift optimization (gcc should do that ...)
>  #define NEG_SSR32 NEG_SSR32
>  static inline  int32_t NEG_SSR32( int32_t a, int8_t s){
> +    if (__builtin_constant_p(s))
>      __asm__ ("sarl %1, %0\n\t"
>           : "+r" (a)
> -         : "ic" ((uint8_t)(-s))
> +         : "i" (-s & 0x1F)
>      );
> +    else
> +        __asm__ ("sarl %1, %0\n\t"
> +               : "+r" (a)
> +               : "c" ((uint8_t)(-s))
> +        );
>      return a;
>  }
>  
>  #define NEG_USR32 NEG_USR32
>  static inline uint32_t NEG_USR32(uint32_t a, int8_t s){
> +    if (__builtin_constant_p(s))
>      __asm__ ("shrl %1, %0\n\t"
>           : "+r" (a)
> -         : "ic" ((uint8_t)(-s))
> +         : "i" (-s & 0x1F)
>      );
> +    else
> +        __asm__ ("shrl %1, %0\n\t"
> +               : "+r" (a)
> +               : "c" ((uint8_t)(-s))
> +        );
>      return a;
>  }
>  

Does this have a performance or codesize impact?
And is the inline ASM actually any good? (When I comment the inline ASM
of NEG_USR32 out, code size actually increases with GCC 11, suggesting
that the inline ASM may be counterproductive as it impairs the compilers
ability to optimize.)

- Andreas

On 7/16/2023 1:00 PM, Andreas Rheinhardt wrote:
> James Almer:
>> From: Rémi Denis-Courmont <remi@remlab.net>
>>
>> Fixes assembling with binutil as >= 2.41
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/x86/mathops.h | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
>> index 6298f5ed19..ca7e2dffc1 100644
>> --- a/libavcodec/x86/mathops.h
>> +++ b/libavcodec/x86/mathops.h
>> @@ -35,12 +35,20 @@
>>   static av_always_inline av_const int MULL(int a, int b, unsigned shift)
>>   {
>>       int rt, dummy;
>> +    if (__builtin_constant_p(shift))
> 
> We actually have av_builtin_constant_p. Is it guaranteed that all
> compilers supporting inline ASM also support __builtin_constant_p?

I can use av_builtin_constant_p() if you want, but it will be expanded 
to __builtin_constant_p() in all supported compilers, judging by how 
it's defined.

> 
>>       __asm__ (
>>           "imull %3               \n\t"
>>           "shrdl %4, %%edx, %%eax \n\t"
>>           :"=a"(rt), "=d"(dummy)
>> -        :"a"(a), "rm"(b), "ci"((uint8_t)shift)
>> +        :"a"(a), "rm"(b), "i"(shift & 0x1F)
>>       );
>> +    else
>> +        __asm__ (
>> +            "imull %3               \n\t"
>> +            "shrdl %4, %%edx, %%eax \n\t"
>> +            :"=a"(rt), "=d"(dummy)
>> +            :"a"(a), "rm"(b), "c"((uint8_t)shift)
>> +        );
>>       return rt;
>>   }
>>   
>> @@ -113,19 +121,31 @@ __asm__ volatile(\
>>   // avoid +32 for shift optimization (gcc should do that ...)
>>   #define NEG_SSR32 NEG_SSR32
>>   static inline  int32_t NEG_SSR32( int32_t a, int8_t s){
>> +    if (__builtin_constant_p(s))
>>       __asm__ ("sarl %1, %0\n\t"
>>            : "+r" (a)
>> -         : "ic" ((uint8_t)(-s))
>> +         : "i" (-s & 0x1F)
>>       );
>> +    else
>> +        __asm__ ("sarl %1, %0\n\t"
>> +               : "+r" (a)
>> +               : "c" ((uint8_t)(-s))
>> +        );
>>       return a;
>>   }
>>   
>>   #define NEG_USR32 NEG_USR32
>>   static inline uint32_t NEG_USR32(uint32_t a, int8_t s){
>> +    if (__builtin_constant_p(s))
>>       __asm__ ("shrl %1, %0\n\t"
>>            : "+r" (a)
>> -         : "ic" ((uint8_t)(-s))
>> +         : "i" (-s & 0x1F)
>>       );
>> +    else
>> +        __asm__ ("shrl %1, %0\n\t"
>> +               : "+r" (a)
>> +               : "c" ((uint8_t)(-s))
>> +        );
>>       return a;
>>   }
>>   
> 
> Does this have a performance or codesize impact?

It should behave the same it has until now.

> And is the inline ASM actually any good? (When I comment the inline ASM
> of NEG_USR32 out, code size actually increases with GCC 11, suggesting
> that the inline ASM may be counterproductive as it impairs the compilers
> ability to optimize.)

I did not test nor check if removing this ricing is better or not. It 
can be looked at later. Right now, i want lavc to compile with binutils 2.41

> 
> - Andreas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Le sunnuntaina 16. heinäkuuta 2023, 19.00.35 EEST Andreas Rheinhardt a écrit :
> > @@ -113,19 +121,31 @@ __asm__ volatile(\
> > 
> >  // avoid +32 for shift optimization (gcc should do that ...)
> >  #define NEG_SSR32 NEG_SSR32
> >  static inline  int32_t NEG_SSR32( int32_t a, int8_t s){
> > 
> > +    if (__builtin_constant_p(s))
> > 
> >      __asm__ ("sarl %1, %0\n\t"
> >      
> >           : "+r" (a)
> > 
> > -         : "ic" ((uint8_t)(-s))
> > +         : "i" (-s & 0x1F)
> > 
> >      );
> > 
> > +    else
> > +        __asm__ ("sarl %1, %0\n\t"
> > +               : "+r" (a)
> > +               : "c" ((uint8_t)(-s))
> > +        );
> > 
> >      return a;
> >  
> >  }
> >  
> >  #define NEG_USR32 NEG_USR32
> >  static inline uint32_t NEG_USR32(uint32_t a, int8_t s){
> > 
> > +    if (__builtin_constant_p(s))
> > 
> >      __asm__ ("shrl %1, %0\n\t"
> >      
> >           : "+r" (a)
> > 
> > -         : "ic" ((uint8_t)(-s))
> > +         : "i" (-s & 0x1F)
> > 
> >      );
> > 
> > +    else
> > +        __asm__ ("shrl %1, %0\n\t"
> > +               : "+r" (a)
> > +               : "c" ((uint8_t)(-s))
> > +        );
> > 
> >      return a;
> >  
> >  }
> 
> Does this have a performance or codesize impact?

It performs the masking at compilation time rather than run-time, thus saving 
one masking instruction and sparing an implicit clobber on ECX.

However, since we are dealing with constants, we the shift amount could 
*presumably* just as well be fixed in the calling code. Not that I'd know.

> And is the inline ASM actually any good?

To be honest, even if it's good, it inhibits instruction scheduling by the 
compiler. So IMO the threshold should be for assembler to be strictly *better* 
than the C code.

(And I don't know the answer to that question.)