libvisual icon indicating copy to clipboard operation
libvisual copied to clipboard

MMX implementation of LV::Video's blit_overlay_alphasrc() is broken

Open kaixiong opened this issue 2 years ago • 5 comments

VideoBlit::blit_overlay_alphasrc_mmx() is the SIMD implementation of _VideoBlit::blit_overlay_alphasrc() using x86 MMX instructions. The MMX register mm6 is used for an unpack but its value is never iniitalized. Could be a confusion due to a translation into AT&T syntax.

movd %[spix], %%mm0
movd %[dpix], %%mm1
movq %%mm0, %%mm2
movq %%mm0, %%mm3
psrlq $24, %%mm2
movq %%mm0, %%mm4
psrld $24, %%mm3
psrld $24, %%mm4
psllq $32, %%mm2
psllq $16, %%mm3
por %%mm4, %%mm2
punpcklbw %%mm6, %%mm0    ; <-- mm6 is never initialized (mm0 is destination, mm6 is source)
por %%mm3, %%mm2
punpcklbw %%mm6, %%mm1    ; <-- mm6 is never initialized (mm0 is destination, mm6 is source)
psubsw %%mm1, %%mm0
pmullw %%mm2, %%mm0
psrlw $8, %%mm0
paddb %%mm1, %%mm0
packuswb %%mm0, %%mm0
movd %%mm0, %[dest]

kaixiong avatar Jan 31 '23 22:01 kaixiong

Consider rewriting this using SIMD intrinsics. Its equivalent intrinsic _mm_unpackhi_pi16 returns the result, rather than modifying one operand in place.

Or use ORC, if it's possible.

kaixiong avatar Jan 31 '23 22:01 kaixiong

I think ORC is a bit overkill.

However writing this in SIMD intrinsics would be great, I am no longer familiair with the code, but I am sure there were some prefetchnta's or alikes nearby, those are probably important for good throughput.

dsmit avatar Feb 02 '23 14:02 dsmit

@dsmit, yeah you're right. Prefetch only exists from SSE and onwards though.

I rewrote the MMX code using intrinsics and ran it through Godbolt. Seems like GCC turns them into SSE using XMM registers anyway. Clang keeps the use of MM registers but throws in SSE shuffling instructions.

Maybe it's time to drop MMX and use SSE2. SSE2 was introduced in 2000 to P4s.

kaixiong avatar Feb 02 '23 21:02 kaixiong

I think 3DNow! had some prefetch instructions, and we've used those, although this is a long time ago, and stuff I haven't been working with for over a decade.

But I'd drop MMX in a heartbeat and move to 128/256 wide SIMD.

I didn't know using intrinsics gave such wild variations between implementations.

dsmit avatar Feb 03 '23 09:02 dsmit

@dsmit, it's probably because x86-64 GCC and Clang targets SSE by default. Since SSE is a superset of MMX, the compiler is free to use wider registers and additional instructions to achieve the same results.

kaixiong avatar Feb 05 '23 02:02 kaixiong