Libvisual / libvisual

Libvisual Audio Visualization
http://libvisual.org/
83 stars 31 forks source link

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

Open kaixiong opened 1 year ago

kaixiong commented 1 year ago

_VideoBlit::blit_overlay_alphasrcmmx() 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 commented 1 year ago

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.

dsmit commented 1 year ago

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.

kaixiong commented 1 year ago

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

dsmit commented 1 year ago

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.

kaixiong commented 1 year ago

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