[Pixman] [PATCH 2/6] mmx: compile on MIPS for Loongson MMI optimizations

[Matt Turner]

On Tue, Feb 28, 2012 at 8:57 PM, Søren Sandmann <sandmann at cs.au.dk> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>
>> +/* vectors are stored in 64-bit floating-point registers */
>> +typedef double __m64;
>
>> [...]
>
>> @@ -114,11 +118,14 @@ _mm_shuffle_pi16 (__m64 __A, int8_t const __N)
>>   * uint64_t and __m64 values, then define USE_CVT_INTRINSICS.
>>   * If __m64 and uint64_t values can just be cast to each other directly,
>>   * then define USE_M64_CASTS.
>> + * If __m64 is a double datatype, then define USE_M64_DOUBLE.
>>   */
>>  #ifdef _MSC_VER
>>  # define M64_MEMBER m64_u64
>>  #elif defined(__ICC)
>>  # define USE_CVT_INTRINSICS
>> +#elif defined(USE_LOONGSON_MMI)
>> +# define USE_M64_DOUBLE
>>  #elif defined(__GNUC__)
>>  # define USE_M64_CASTS
>>  #elif defined(__SUNPRO_C)
>> @@ -136,7 +143,7 @@ _mm_shuffle_pi16 (__m64 __A, int8_t const __N)
>>  # endif
>>  #endif
>>
>> -#if defined(USE_M64_CASTS) || defined(USE_CVT_INTRINSICS)
>> +#if defined(USE_M64_CASTS) || defined(USE_CVT_INTRINSICS) || defined(USE_M64_DOUBLE)
>>  typedef uint64_t mmxdatafield;
>>  #else
>>  typedef __m64 mmxdatafield;
>> @@ -188,6 +195,8 @@ static const mmx_data_t c =
>>  #    define MC(x) to_m64 (c.mmx_ ## x)
>>  #elif defined(USE_M64_CASTS)
>>  #    define MC(x) ((__m64)c.mmx_ ## x)
>> +#elif defined(USE_M64_DOUBLE)
>> +#    define MC(x) (*(__m64 *)&c.mmx_ ## x)
>>  #else
>>  #    define MC(x) c.mmx_ ## x
>>  #endif
>
> As far as I can tell, in the above mmxdatafield is defined to be a
> uint64_t, and MC is defined to cast from uint64_t to __m64, which is a
> double. Is there any reason to not just define mmxdatafield as double
> and then drop the casting?

I couldn't figure out a way to assign raw hex values to doubles.
Assigning 0x00ff00ff00ff00ff to double interprets the hex as an
integer and then converts it to a double.

>> @@ -347,13 +360,26 @@ static __inline__ uint32_t ldl_u(uint32_t *p)
>>  static force_inline __m64
>>  load (const uint32_t *v)
>>  {
>> +#ifdef USE_LOONGSON_MMI
>> +    __m64 ret;
>> +    asm("lwc1 %0, %1\n\t"
>> +     : "=f" (ret)
>> +     : "m" (*v)
>> +    );
>> +    return ret;
>
> Please put a space before "(". There are several instances of this, in
> this and the other patches, and also a lot of existing cases in
> pixman-mmx.c that weren't introduced by this series. Since this series
> changes all the ldq_u() calls in the first patch, we might as well get
> all of it cleaned up.

Will do.

>> +#else
>>      return _mm_cvtsi32_si64 (*v);
>> +#endif
>>  }
>>
>>  static force_inline __m64
>>  load8888 (const uint32_t *v)
>>  {
>> +#ifdef USE_LOONGSON_MMI
>> +    return _mm_unpacklo_pi8_f (*(__m32 *)v, _mm_setzero_si64 ());
>> +#else
>>      return _mm_unpacklo_pi8 (load (v), _mm_setzero_si64 ());
>> +#endif
>>  }
>
> Above you defined what load() does on Loongson, but then you don't use
> it here? It doesn't look like load() is used elsewhere, though I could
> have overlooked some.

Odd. I have different versions of patch 3 on my amd64 and loongson
systems. The one on the loongson system uses load(), but it's not
actually necessary. I'll drop the loongson-specific load()-code in
this patch until a time when it's needed.

Thanks,
Matt