Re: [patch, spu] Implementation of vector unpack for Cell SPU

[Ira Rosen <IRAR at il dot ibm dot com>]

Hi Trevor,

Trevor_Smigiel@playstation.sony.com wrote on 23/01/2008 07:00:18:

> Ira,
>
> See the five comments below.  They all apply to each of the similar
> patterns.

I changed the patch according to your comments. I removed vec_extendhisi2
and used spu_xshw instead. I also used vec_select in vec_unpacks, since
otherwise I got "warning: this decimal constant is unsigned only in ISO
C90".

Tested on Cell SPU. O.K. for 4.4?

Thanks,
Ira

ChangeLog:

      * config/spu/spu.md (vec_unpacku_hi_v8hi, vec_unpacku_lo_v8hi,
      vec_unpacks_hi_v8hi, vec_unpacks_lo_v8hi, vec_unpacku_hi_v16qi,
      vec_unpacku_lo_v16qi, vec_unpacks_lo_v16qi): Implement.

testsuite/ChangeLog:

      * lib/target-supports.exp (check_effective_target_vect_unpack):
      Return true for SPU.
      (check_effective_target_vect_short_mult): Likewise.
      * gcc.dg/vect/vect-reduc-dot-s16b.c: Expect vectorization of
      the loop on targets that support vect_unpack.

(See attached file: unpack.txt)

>
> > Index: config/spu/spu.md
> > ===================================================================
> > --- config/spu/spu.md   (revision 131146)
> > +++ config/spu/spu.md   (working copy)
> > @@ -153,6 +153,14 @@
> >   (UNSPEC_SPU_REALIGN_LOAD 49)
> >   (UNSPEC_SPU_MASK_FOR_LOAD 50)
> >   (UNSPEC_DFTSV          51)
> > + (UNSPEC_VUPKHUH         52)
> > + (UNSPEC_VUPKLUH         53)
> > + (UNSPEC_VUPKHSH         54)
> > + (UNSPEC_VUPKLSH         55)
> > + (UNSPEC_VUPKHUB        56))
> > + (UNSPEC_VUPKLUB         57)
> > + (UNSPEC_VUPKHSB         58)
> > + (UNSPEC_VUPKLSB         59)
>
> These aren't necessary, they are only used in expands.  Remove them and
> just use -1 in the expands.
>
> >  ])
> >
> >  (include "predicates.md")
> > @@ -4403,3 +4411,159 @@ selb\t%0,%4,%0,%3"
> >
> >    DONE;,
> >  }")
> > +}
> > +(define_expand "vec_unpacku_hi_v8hi"
> > +  [(set (match_operand:V4SI 0 "register_operand" "=r")
> > +        (unspec:V4SI [(match_operand:V8HI 1 "register_operand" "r")]
>
> Use spu_reg_operand instead of register_operand in all cases.
>
> > +                     UNSPEC_VUPKHUH))]
> > +  ""
> > +  "
>      ^
> As a matter of style, please don't add this unnecessary quote '"'...
>
> > +{
> > +  rtx mask = gen_reg_rtx (TImode);
> > +  unsigned char arr[16] = {
> > +    0x80, 0x80, 0x00, 0x01, 0x80, 0x80, 0x02, 0x03,
> > +    0x80, 0x80, 0x04, 0x05, 0x80, 0x80, 0x06, 0x07};
> > +
> > +  emit_move_insn (mask, array_to_constant (TImode, arr));
> > +  emit_insn (gen_shufb (operands[0], operands[1], operands[1], mask));
> > +
> > +  DONE;
> > +}")
>     ^
> or this one.
>
> > +}
>    ^
> Is this supposed to be here?  It looks like an extra closing brace.
> Sorry, I have't tried applying the patch yet, which might have answered
> this question for me.
>
> > +(define_expand "vec_unpacku_lo_v8hi"
> > +  [(set (match_operand:V4SI 0 "register_operand" "=r")
> > +        (unspec:V4SI [(match_operand:V8HI 1 "register_operand" "r")]);
> > +                     UNSPEC_VUPKLUH))]
> > +  ""
> > +  "
> > +{
> > +  rtx mask = gen_reg_rtx (TImode);
> > +  unsigned char arr[16] = {
> > +    0x80, 0x80, 0x08, 0x09, 0x80, 0x80, 0x0A, 0x0B,
> > +    0x80, 0x80, 0x0C, 0x0D, 0x80, 0x80, 0x0E, 0x0F};
> > +
> > +  emit_move_insn (mask, array_to_constant (TImode, arr));
> > +  emit_insn (gen_shufb (operands[0], operands[1], operands[1], mask));
> > +
> > +  DONE;
> > +}")
> > +}
> > +(define_insn "vec_extendhisi2"
> > +  [(set (match_operand:V4SI 0 "register_operand" "=r")
> > +        (sign_extend:V4SI (match_operand:V4SI 1 "register_operand"
"r")))]
>
> Operand 1 should be mode V4HI.  This means using a vec_select, like the
> vec_widen* patterns.
>
>
> > +  ""
> > +  "xshw\t%0,%1")
>
> [ ... ]
>
> Trevor
>
>

Attachment: unpack.txt
Description: Text document