[FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[James Almer]

Should fix assembling with binutil as >= 2.41

Signed-off-by: James Almer <jamrial@gmail.com>
---
This is IMO a big breakage. binutil's as has until now clipped these values on
its own, and never required the compiler to do it.

 libavcodec/x86/mathops.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libavcodec/x86/mathops.h b/libavcodec/x86/mathops.h
index 6298f5ed19..a08c6193bf 100644
--- a/libavcodec/x86/mathops.h
+++ b/libavcodec/x86/mathops.h
@@ -39,7 +39,7 @@ static av_always_inline av_const int MULL(int a, int b, unsigned shift)
         "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), "cI"((uint8_t)shift)
     );
     return rt;
 }
@@ -115,16 +115,17 @@ __asm__ volatile(\
 static inline  int32_t NEG_SSR32( int32_t a, int8_t s){
     __asm__ ("sarl %1, %0\n\t"
          : "+r" (a)
-         : "ic" ((uint8_t)(-s))
+         : "Ic" ((uint8_t)(-s))
     );
     return a;
 }
 
 #define NEG_USR32 NEG_USR32
 static inline uint32_t NEG_USR32(uint32_t a, int8_t s){
+
     __asm__ ("shrl %1, %0\n\t"
          : "+r" (a)
-         : "ic" ((uint8_t)(-s))
+         : "Ic" ((uint8_t)(-s))
     );
     return a;
 }
-- 
2.41.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[Rémi Denis-Courmont]

Le sunnuntaina 16. heinäkuuta 2023, 2.58.32 EEST James Almer a écrit :
> Should fix assembling with binutil as >= 2.41
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This is IMO a big breakage. binutil's as has until now clipped these values
> on its own, and never required the compiler to do it.

TBH, silently clipping immediate constants sounds like a nasty bug that could 
cause really nasty suprises if somebody every passes an out-of-range constant. 
This has happened to me many times, typically with incidentally out-of-range 
immediate offsets in loads/stores.

(...)

>      __asm__ ("shrl %1, %0\n\t"
>           : "+r" (a)
> -         : "ic" ((uint8_t)(-s))
> +         : "Ic" ((uint8_t)(-s))

Note that this is not equivalent. Now, if `s` is constant but out of range, 
the compiler will be required to fit it. And it does that by moving it into 
ECX. This is probably not what you want.

AFAICT, you should keep the constraint as it is, and fix the operand value 
instead by masking it, e.g.:

        if (__builtin_constant_p(s))
                __asm__ ("shrl %1, %0\n\t"
                        : "+r" (a)
                        : "i" ((-s) & 0x1f)
                );
        else
                __asm__ ("shrl %1, %0\n\t"
                        : "+r" (a)
                        : "c" (-s)
                );

(Not sure if the the 0x1f mask is correct, but you get the idea.)

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



_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[Nicolas George]

[-- Attachment #1.1: Type: text/plain, Size: 1219 bytes --]

James Almer (12023-07-15):
> Should fix assembling with binutil as >= 2.41
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This is IMO a big breakage. binutil's as has until now clipped these values on
> its own, and never required the compiler to do it.

I confirm it fixes the build failures on up-to-date Debian testing.

OTOH, I ran a benchmark (decoding some x264):

474134     mod
488751 orig
494359     mod
498554 orig
508958 orig
514246 orig
518160     mod
528427 orig
530223     mod
534762     mod
536415 orig
548434 orig
550789 orig
551716     mod
553951 orig
561754 orig
572688     mod
580254     mod
581205 orig
583856     mod
583939 orig
584748 orig
594143 orig
600681     mod
607596     mod
612757     mod
621033 orig
624567 orig
626346     mod
627309     mod
628242     mod
638344     mod

The numbers are the sum of the “user” column of the -benchmark_all
output, on an AMD Ryzen 3 3200U and Debaian stable. The mod lines are
when I disabled the two faulty functions.

They are all over the place, it is hard to be sure, but it seems to
indicate that, as you suspected, the benefit is not that big.

Regards,

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[James Almer]

On 7/16/2023 6:23 AM, Rémi Denis-Courmont wrote:
> Le sunnuntaina 16. heinäkuuta 2023, 2.58.32 EEST James Almer a écrit :
>> Should fix assembling with binutil as >= 2.41
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This is IMO a big breakage. binutil's as has until now clipped these values
>> on its own, and never required the compiler to do it.
> 
> TBH, silently clipping immediate constants sounds like a nasty bug that could
> cause really nasty suprises if somebody every passes an out-of-range constant.

We're passing it out or range constants alright. I tried adding an 
av_assert0((uint8_t)(-s) <= 31) and most fate tests started failing.

> This has happened to me many times, typically with incidentally out-of-range
> immediate offsets in loads/stores.
> 
> (...)
> 
>>       __asm__ ("shrl %1, %0\n\t"
>>            : "+r" (a)
>> -         : "ic" ((uint8_t)(-s))
>> +         : "Ic" ((uint8_t)(-s))
> 
> Note that this is not equivalent. Now, if `s` is constant but out of range,
> the compiler will be required to fit it. And it does that by moving it into
> ECX. This is probably not what you want.
> 
> AFAICT, you should keep the constraint as it is, and fix the operand value
> instead by masking it, e.g.:
> 
>          if (__builtin_constant_p(s))
>                  __asm__ ("shrl %1, %0\n\t"
>                          : "+r" (a)
>                          : "i" ((-s) & 0x1f)
>                  );
>          else
>                  __asm__ ("shrl %1, %0\n\t"
>                          : "+r" (a)
>                          : "c" (-s)
>                  );
> 
> (Not sure if the the 0x1f mask is correct, but you get the idea.)

It is, just tested.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/x86/mathops: use constrained immediate operands

[Rémi Denis-Courmont]

Le sunnuntaina 16. heinäkuuta 2023, 14.55.43 EEST James Almer a écrit :
> On 7/16/2023 6:23 AM, Rémi Denis-Courmont wrote:
> > Le sunnuntaina 16. heinäkuuta 2023, 2.58.32 EEST James Almer a écrit :
> >> Should fix assembling with binutil as >= 2.41
> >> 
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> This is IMO a big breakage. binutil's as has until now clipped these
> >> values
> >> on its own, and never required the compiler to do it.
> > 
> > TBH, silently clipping immediate constants sounds like a nasty bug that
> > could cause really nasty suprises if somebody every passes an
> > out-of-range constant.
> We're passing it out or range constants alright. I tried adding an
> av_assert0((uint8_t)(-s) <= 31) and most fate tests started failing.

Well, yes. That's why recent binutils is complaining. It wouldn't if the 
constant values were always in range.

I'm not versed in the x86 subdomain of black magic, so I'm not sure if you 
imply that it was intentional that FFmpeg fed out of range values that would 
be cropped, or if it was unintentional. In the later case, I think that the 
existing assembler constraint should actually be kept as it is precisely to 
detect errors, and the calling code path ought to be fixed instead.

Either way, changing "i" for "I" will generate suboptimal-looking code as I 
pointed out up-thread. If we don't even care about that, then we migth as well 
shift in C code, AFAICT.

> > This has happened to me many times, typically with incidentally
> > out-of-range immediate offsets in loads/stores.
> > 
> > (...)
> > 
> >>       __asm__ ("shrl %1, %0\n\t"
> >>       
> >>            : "+r" (a)
> >> 
> >> -         : "ic" ((uint8_t)(-s))
> >> +         : "Ic" ((uint8_t)(-s))
> > 
> > Note that this is not equivalent. Now, if `s` is constant but out of
> > range,
> > the compiler will be required to fit it. And it does that by moving it
> > into
> > ECX. This is probably not what you want.
> > 
> > AFAICT, you should keep the constraint as it is, and fix the operand value
> > 
> > instead by masking it, e.g.:
> >          if (__builtin_constant_p(s))
> >          
> >                  __asm__ ("shrl %1, %0\n\t"
> >                  
> >                          : "+r" (a)
> >                          : "i" ((-s) & 0x1f)
> >                  
> >                  );
> >          
> >          else
> >          
> >                  __asm__ ("shrl %1, %0\n\t"
> >                  
> >                          : "+r" (a)
> >                          : "c" (-s)
> >                  
> >                  );
> > 
> > (Not sure if the the 0x1f mask is correct, but you get the idea.)
> 
> It is, just tested.
> _______________________________________________
> 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".


-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



_______________________________________________
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".