| Message ID | 20240126130427.2159537-1-martin@martin.st |
|---|---|
| State | Accepted |
| Commit | 7ec2354c38978b918dc079b611393becb6c80bf7 |
| Headers | show |
| Series | [FFmpeg-devel] x86: Remove inline MMX assembly that clobbers the FPU state | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Fri, 26 Jan 2024, Martin Storsjö wrote: > These inline implementations of AV_COPY64, AV_SWAP64 and AV_ZERO64 > are known to clobber the FPU state - which has to be restored > with the 'emms' instruction afterwards. > > This was known and signaled with the FF_COPY_SWAP_ZERO_USES_MMX > define, which calling code seems to have been supposed to check, > in order to call emms_c() after using them. See > 0b1972d4096df5879038f0af776f87f41e90ebd4, > 29c4c0886d143790fcbeddbe40a23dfc6f56345c and > df215e575850e41b19aeb1fd99e53372a6b3d537 for history on earlier > fixes in the same area. > > However, new code can use these AV_*64() macros without knowing > about the need to call emms_c(). > > Just get rid of these dangerous inline assembly snippets; this > doesn't make any difference for 64 bit architectures anyway. > > Signed-off-by: Martin Storsjö <martin@martin.st> > --- > libavcodec/dca_core.c | 16 ---------------- > libavutil/x86/intreadwrite.h | 36 ------------------------------------ > 2 files changed, 52 deletions(-) I forgot to add some more context here; the VVC tests fail on i386 in some cases. https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240125170518.61211-1-post@frankplowman.com/ fixes this, by using av_log2() instead of the float log2() in the VVC decoder. This patch fixes the same issue as well, by eliminating the FPU state clobbering (so that float math functions anywhere in decoders work as expected). // Martin
On Fri, 26 Jan 2024, Martin Storsjö wrote: > On Fri, 26 Jan 2024, Martin Storsjö wrote: > >> These inline implementations of AV_COPY64, AV_SWAP64 and AV_ZERO64 >> are known to clobber the FPU state - which has to be restored >> with the 'emms' instruction afterwards. >> >> This was known and signaled with the FF_COPY_SWAP_ZERO_USES_MMX >> define, which calling code seems to have been supposed to check, >> in order to call emms_c() after using them. See >> 0b1972d4096df5879038f0af776f87f41e90ebd4, >> 29c4c0886d143790fcbeddbe40a23dfc6f56345c and >> df215e575850e41b19aeb1fd99e53372a6b3d537 for history on earlier >> fixes in the same area. >> >> However, new code can use these AV_*64() macros without knowing >> about the need to call emms_c(). >> >> Just get rid of these dangerous inline assembly snippets; this >> doesn't make any difference for 64 bit architectures anyway. >> >> Signed-off-by: Martin Storsjö <martin@martin.st> >> --- >> libavcodec/dca_core.c | 16 ---------------- >> libavutil/x86/intreadwrite.h | 36 ------------------------------------ >> 2 files changed, 52 deletions(-) > > I forgot to add some more context here; the VVC tests fail on i386 in some > cases. > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240125170518.61211-1-post@frankplowman.com/ > fixes this, by using av_log2() instead of the float log2() in the VVC > decoder. This patch fixes the same issue as well, by eliminating the FPU > state clobbering (so that float math functions anywhere in decoders work as > expected). If there are no better suggestions here, I would like to go ahead and push this. // Martin
diff --git a/libavcodec/dca_core.c b/libavcodec/dca_core.c index 60508fabb9..5dd727fc72 100644 --- a/libavcodec/dca_core.c +++ b/libavcodec/dca_core.c @@ -770,10 +770,6 @@ static void erase_adpcm_history(DCACoreDecoder *s) for (ch = 0; ch < DCA_CHANNELS; ch++) for (band = 0; band < DCA_SUBBANDS; band++) AV_ZERO128(s->subband_samples[ch][band] - DCA_ADPCM_COEFFS); - -#ifdef FF_COPY_SWAP_ZERO_USES_MMX - emms_c(); -#endif } static int alloc_sample_buffer(DCACoreDecoder *s) @@ -837,10 +833,6 @@ static int parse_frame_data(DCACoreDecoder *s, enum HeaderType header, int xch_b } } -#ifdef FF_COPY_SWAP_ZERO_USES_MMX - emms_c(); -#endif - return 0; } @@ -1283,10 +1275,6 @@ static void erase_x96_adpcm_history(DCACoreDecoder *s) for (ch = 0; ch < DCA_CHANNELS; ch++) for (band = 0; band < DCA_SUBBANDS_X96; band++) AV_ZERO128(s->x96_subband_samples[ch][band] - DCA_ADPCM_COEFFS); - -#ifdef FF_COPY_SWAP_ZERO_USES_MMX - emms_c(); -#endif } static int alloc_x96_sample_buffer(DCACoreDecoder *s) @@ -1516,10 +1504,6 @@ static int parse_x96_frame_data(DCACoreDecoder *s, int exss, int xch_base) } } -#ifdef FF_COPY_SWAP_ZERO_USES_MMX - emms_c(); -#endif - return 0; } diff --git a/libavutil/x86/intreadwrite.h b/libavutil/x86/intreadwrite.h index 40f375b013..5e57d6a8cd 100644 --- a/libavutil/x86/intreadwrite.h +++ b/libavutil/x86/intreadwrite.h @@ -27,42 +27,6 @@ #if HAVE_MMX -#if !HAVE_FAST_64BIT && defined(__MMX__) - -#define FF_COPY_SWAP_ZERO_USES_MMX - -#define AV_COPY64 AV_COPY64 -static av_always_inline void AV_COPY64(void *d, const void *s) -{ - __asm__("movq %1, %%mm0 \n\t" - "movq %%mm0, %0 \n\t" - : "=m"(*(uint64_t*)d) - : "m" (*(const uint64_t*)s) - : "mm0"); -} - -#define AV_SWAP64 AV_SWAP64 -static av_always_inline void AV_SWAP64(void *a, void *b) -{ - __asm__("movq %1, %%mm0 \n\t" - "movq %0, %%mm1 \n\t" - "movq %%mm0, %0 \n\t" - "movq %%mm1, %1 \n\t" - : "+m"(*(uint64_t*)a), "+m"(*(uint64_t*)b) - ::"mm0", "mm1"); -} - -#define AV_ZERO64 AV_ZERO64 -static av_always_inline void AV_ZERO64(void *d) -{ - __asm__("pxor %%mm0, %%mm0 \n\t" - "movq %%mm0, %0 \n\t" - : "=m"(*(uint64_t*)d) - :: "mm0"); -} - -#endif /* !HAVE_FAST_64BIT && defined(__MMX__) */ - #ifdef __SSE__ #define AV_COPY128 AV_COPY128
These inline implementations of AV_COPY64, AV_SWAP64 and AV_ZERO64 are known to clobber the FPU state - which has to be restored with the 'emms' instruction afterwards. This was known and signaled with the FF_COPY_SWAP_ZERO_USES_MMX define, which calling code seems to have been supposed to check, in order to call emms_c() after using them. See 0b1972d4096df5879038f0af776f87f41e90ebd4, 29c4c0886d143790fcbeddbe40a23dfc6f56345c and df215e575850e41b19aeb1fd99e53372a6b3d537 for history on earlier fixes in the same area. However, new code can use these AV_*64() macros without knowing about the need to call emms_c(). Just get rid of these dangerous inline assembly snippets; this doesn't make any difference for 64 bit architectures anyway. Signed-off-by: Martin Storsjö <martin@martin.st> --- libavcodec/dca_core.c | 16 ---------------- libavutil/x86/intreadwrite.h | 36 ------------------------------------ 2 files changed, 52 deletions(-)