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

[Rémi Denis-Courmont]

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

-- 
Rémi Denis-Courmont
http://www.remlab.net/