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

[Andreas Rheinhardt]

James Almer:
> From: Rémi Denis-Courmont <remi at remlab.net>
> 
> Fixes assembling with binutil as >= 2.41
> 
> Signed-off-by: James Almer <jamrial at 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