[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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


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