RE: [PATCH, ARM, iWMMXt][2/5]: intrinsic head file change

[Xinyu Qi <xyqi at marvell dot com>]

At 2011-08-18 09:33:27,"Ramana Radhakrishnan" <ramana.radhakrishnan@linaro.org> wrote:
> On 6 July 2011 11:11, Xinyu Qi <xyqi@marvell.com> wrote:
> > Hi,
> >
> > It is the second part of iWMMXt maintenance.
> >
> > *config/arm/mmintrin.h:
> > ?Revise the iWMMXt intrinsics head file. Fix some intrinsics and add some
> new intrinsics
> 
> Is there a document somewhere that lists these intrinsics and what
> each of these are supposed to be doing ? Missing details again . We
> seem to be changing quite a few things.

Hi,
The intrinsic_doc.txt is attached. It is the piece of iWMMXt intrinsic details doc picked out from "Intel Wireless MMX Technology Intrinsic Support" with some modification.

> > +
> > +/*  We will treat __int64 as a long long type
> > +    and __m64 as an unsigned long long type to conform to VSC++.  */Is
> > +typedef unsigned long long __m64;
> > +typedef long long __int64;
> 
> Interesting this sort of a change with these cases where you are
> changing the type to conform to VSC++ ? This just means old code that
> uses this is pretty much broken. Not that I have much hope of that
> happening by default - -flax-conversions appears to be needed even
> with a trunk compiler.

I couldn't find any material to show why __int64 needs to be redefined. And all the tests are passed without this change. So decide to discard this change.

> 
> > @@ -54,7 +63,7 @@ _mm_cvtsi64_si32 (__int64 __i)
> >  static __inline __int64
> >  _mm_cvtsi32_si64 (int __i)
> >  {
> > -  return __i;
> > +  return (__i & 0xffffffff);
> >  }
> 
> Eh ? why the & 0xffffffff before promotion rules.  Is this set of
> intrinsics documented some place ?  What is missing and could be the
> subject of a follow-up patch is a set of tests for the wMMX intrinsics
> ....

See the intrinsics doc. It says the description of _mm_cvtsi32_si64 is "The integer value is zero-extended to 64 bits.
If r = _mm_cvtsi32_si64(i), then the action is
r [0:31] = i;
r[32:63] = 0;"

> 
> What's the behaviour of wandn supposed to be ? Does wandn x, y, z
> imply x = y & ~z or x = ~y & z ? If the former then your intrinsic
> expansion is wrong unless the meaning of this has changed ? Whats the
> behaviour of the intrinsic __mm_and_not_si64 . ?

The description of _mm_andnot_si64 is "Performs a logical NOT on the 64-bit value in m1 and use the result in a bitwise AND with the 64-bit value in m2."
And, "wandn wRd, wRn, wRm" means "wRd = wRn & ~wRm"
I think __builtin_arm_wandn had better directly match the behavior of wandn.
Therefore, match _mm_andnot_si64 (m1, m2) to __builtin_arm_wandn (m2, m1).



> @@ -985,44 +1004,83 @@ _mm_setzero_si64 (void)
>  static __inline void
>  _mm_setwcx (const int __value, const int __regno)
>  {
> > +  /*Since gcc has the imformation of all wcgr regs
> > +    in arm backend, use builtin to access them instead
> > +    of throw asm directly.  Thus, gcc could do some
> > +    optimization on them.  */
> > +
> 
> Also this comment is contradictory to what follows in the patch .
> You've prima-facie replaced them with bits of inline assembler. I'm
> not sure this comment makes a lot of sense on its own. 

Sorry. This comment should be removed.

The modified diff is attached.

Thanks,
Xinyu

Attachment: intrinsic_doc.txt
Description: intrinsic_doc.txt

Attachment: 2_mmintrin.diff
Description: 2_mmintrin.diff