[MPlayer-dev-eng] [PATCH] SSE2-optimized libmpeg2 motion compensation

[Guillaume Poirier]

Hi Jim,

jserv at linux2.cc.ntu.edu.tw wrote:
> On Wed, Jun 14, 2006 at 01:48:30PM +0200, Guillaume POIRIER wrote:
>>On 6/14/06, jserv at linux2.cc.ntu.edu.tw <jserv at linux2.cc.ntu.edu.tw> wrote:
>>
>>> Recently, I implement SSE2-optimized libmpeg2 motion compensation, and
>>>I think that it might be useful to MPlayer. I have attached the patch in
>>>this mail.
>>
>>Quick feedback: since you are using intrinsics, you should make your
>>code depend on the availability of mmintrin.h, xmmintrin.h, and
>>emmintrin.h
> 
> 
> I update my patch in attachment, and it should be able to check the 
> availability of MMX intrinsics via the compiler-time definition - 
> HAVE_BUILTIN_VECTOR, which is generated by configure script.

OK, good

>>What CPU did you run your tests on? What compiler did you use?
>>Did you try to see if your code was compiling ok with icc (intel's 
>>compiler)?
> 
> 
> I forgot to mention my hardware:
> 
> $ cat /proc/cpuinfo 
> processor	: 0
> vendor_id	: GenuineIntel
> cpu family	: 6
> model		: 9
> model name	: Intel(R) Pentium(R) M processor 1300MHz
> stepping	: 5
> cpu MHz		: 1293.867
> cache size	: 1024 KB
> 
> I am using GCC 4.1.1, and Intel C++ compiler is known to build with my patch.

Did you check how good of a job do they do at generating good code out
of your intrinsincs?
On a paper I read, GCC-4.1 seemed quite good at intrinsincs and icc
not so good (but so much better at auto-vectorization)

That paper is now at least a year old, so things may have changed. I
suggest you compile the object of your .c file both with icc and with
gcc (leaving the rest of MPlayer as it is) to see if there's a speed
difference...



>  #include "config.h"
> +#include "cpudetect.h"
>  
>  #include <inttypes.h>
>  
> @@ -37,78 +38,22 @@
>  #if defined(ARCH_X86) || defined(ARCH_X86_64)
>  static inline uint32_t arch_accel (void)
>  {
> -    uint32_t eax, ebx, ecx, edx;
> -    int AMD;
> -    uint32_t caps;
> -
> -#if !defined(PIC) && !defined(__PIC__)
> -#define cpuid(op,eax,ebx,ecx,edx)	\
> -    __asm__ ("cpuid"			\
> -	     : "=a" (eax),		\
> -	       "=b" (ebx),		\
> -	       "=c" (ecx),		\
> -	       "=d" (edx)		\
> -	     : "a" (op)			\
> -	     : "cc")
> -#else	/* PIC version : save ebx */
> -#define cpuid(op,eax,ebx,ecx,edx)	\
> -    __asm__ ("push %%ebx\n\t"		\
> -	     "cpuid\n\t"		\
> -	     "movl %%ebx,%1\n\t"	\
> -	     "pop %%ebx"		\
> -	     : "=a" (eax),		\
> -	       "=r" (ebx),		\
> -	       "=c" (ecx),		\
> -	       "=d" (edx)		\
> -	     : "a" (op)			\
> -	     : "cc")
> -#endif
> -
> -    __asm__ ("pushf\n\t"
> -	     "pushf\n\t"
> -	     "pop %0\n\t"
> -	     "movl %0,%1\n\t"
> -	     "xorl $0x200000,%0\n\t"
> -	     "push %0\n\t"
> -	     "popf\n\t"
> -	     "pushf\n\t"
> -	     "pop %0\n\t"
> -	     "popf"
> -	     : "=r" (eax),
> -	       "=r" (ebx)
> -	     :
> -	     : "cc");
> -
> -    if (eax == ebx)		/* no cpuid */
> -	return 0;
> -
> -    cpuid (0x00000000, eax, ebx, ecx, edx);
> -    if (!eax)			/* vendor string only */
> -	return 0;
> -
> -    AMD = (ebx == 0x68747541) && (ecx == 0x444d4163) && (edx == 0x69746e65);
> -
> -    cpuid (0x00000001, eax, ebx, ecx, edx);
> -    if (! (edx & 0x00800000))	/* no MMX */
> -	return 0;
> -
> -    caps = MPEG2_ACCEL_X86_MMX;
> -    if (edx & 0x02000000)	/* SSE - identical to AMD MMX extensions */
> -	caps = MPEG2_ACCEL_X86_MMX | MPEG2_ACCEL_X86_MMXEXT;
> -
> -    cpuid (0x80000000, eax, ebx, ecx, edx);
> -    if (eax < 0x80000001)	/* no extended capabilities */
> -	return caps;
> -
> -    cpuid (0x80000001, eax, ebx, ecx, edx);
> -
> -    if (edx & 0x80000000)
> -	caps |= MPEG2_ACCEL_X86_3DNOW;
> -
> -    if (AMD && (edx & 0x00400000))	/* AMD MMX extensions */
> -	caps |= MPEG2_ACCEL_X86_MMXEXT;
> -
> -    return caps;

I failed to see this chunk last time. Is there a reason why you
removed CPUID support from libmpeg2?
You need to keep in mind that libmpeg2 is an imported library, which
is supposed to be working if you use it in another project.
Therefore, you should not make such changes, because if you do,
libmpeg2 won't work in projects that do not have "general" cpuid check
routines.

What could be done though (in a patch separated from this one) would
be to #ifdef libmpeg2's cpuid code so that if it's used in MPlayer it
doesn't get used.
But as I said, it would have to be done in a different patch.

Guillaume