Re: [PATCH, rs6000] Add support for vec_xst_len_r() and vec_xl_len_r() builtins

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi Carl,

On Wed, Sep 06, 2017 at 08:22:03AM -0700, Carl Love wrote:
> 	(define_insn "*stxvl"): add missing argument to the sldi instruction.

s/add/Add/ .  This one-liner fix is approved right now, please commit
it as a separate patch.


> +(define_insn "addi_neg16"
> +  [(set (match_operand:DI 0 "vsx_register_operand" "=r")
> +       (unspec:DI
> +       [(match_operand:DI 1 "gpc_reg_operand" "r")]
> +       UNSPEC_ADDI_NEG16))]
> +  ""
> +  "addi %0,%1,-16"
> +)

You don't need a separate insn (or unspec) for this at all afaics...
Where you do

  emit_insn (gen_addi_neg16 (tmp, operands[2]));

you could just do

  emit_insn (gen_adddi3 (tmp, operands[2], GEN_INT (-16)));


> +;; Load VSX Vector with Length, right justified
> +(define_expand "lxvll"
> +  [(set (match_dup 3)
> +        (match_operand:DI 2 "register_operand"))
> +   (set (match_operand:V16QI 0 "vsx_register_operand")
> +	(unspec:V16QI
> +	 [(match_operand:DI 1 "gpc_reg_operand")
> +	  (match_dup 3)]
> +	 UNSPEC_LXVLL))]
> +  "TARGET_P9_VECTOR && TARGET_64BIT"
> +{
> +  operands[3] = gen_reg_rtx (DImode);
> +})

Hrm, so you make a reg 3 only because the lxvll pattern will clobber it?

> +(define_insn "*lxvll"
> +  [(set (match_operand:V16QI 0 "vsx_register_operand" "=wa")
> +	(unspec:V16QI
> +	 [(match_operand:DI 1 "gpc_reg_operand" "b")
> +	  (match_operand:DI 2 "register_operand" "+r")]
> +	 UNSPEC_LXVLL))]
> +  "TARGET_P9_VECTOR && TARGET_64BIT"
> +;;  "lxvll %x0,%1,%2;"
> +  "sldi %2,%2, 56\; lxvll %x0,%1,%2;"
> +  [(set_attr "length" "8")
> +   (set_attr "type" "vecload")])

It is nicer to just have a match_scratch in here then, like

(define_insn "*lxvll"
  [(set (match_operand:V16QI 0 "vsx_register_operand" "=wa")
	(unspec:V16QI
	 [(match_operand:DI 1 "gpc_reg_operand" "b")
	  (match_operand:DI 2 "register_operand" "r")]
	 UNSPEC_LXVLL))
   (clobber (match_scratch:DI 3 "=&r"))]
  "TARGET_P9_VECTOR && TARGET_64BIT"
  "sldi %3,%2,56\;lxvll %x0,%1,%3"
  [(set_attr "length" "8")
   (set_attr "type" "vecload")])

(Note spacing, comment, ";" stuff, and the earlyclobber).

Ideally you split the sldi off in the expand though, so that the *lxvll
pattern is really just that single insn.


> +(define_insn "altivec_lvsl_reg"
> +  [(set (match_operand:V16QI 0 "vsx_register_operand" "=v")
> +       (unspec:V16QI
> +       [(match_operand:DI 1 "gpc_reg_operand" "b")]
> +       UNSPEC_LVSL_REG))]
> +  "TARGET_ALTIVEC"
> +  "lvsl %0,0,%1"
> +  [(set_attr "type" "vecload")])

vecload isn't really the correct type for this, but I see we have the
same on the existing lvsl patterns (it's permute unit on p9; I expect
the same on p8 and older, but please check).

Please move this next to the existing lvsl pattern.


> +;; Expand for builtin xl_len_r
> +(define_expand "xl_len_r"
> +  [(match_operand:V16QI 0 "vsx_register_operand" "=v")
> +   (match_operand:DI 1 "register_operand" "r")
> +   (match_operand:DI 2 "register_operand" "r")]
> +  "UNSPEC_XL_LEN_R"
> +{
> +  rtx shift_mask = gen_reg_rtx (V16QImode);
> +  rtx rtx_vtmp = gen_reg_rtx (V16QImode);
> +  rtx tmp = gen_reg_rtx (DImode);
> +
> +/* Setup permute vector to shift right by operands[2] bytes.
> +   Note: addi operands[2], -16 is negative so we actually need to
> +   shift left to get a right shift.  */

Indent the comment with the code, so that's 2 spaces more here.

The comment isn't clear to me...  Neither is the code though: lvsl
looks at just the low 4 bits of its arg, so the addi does nothing
useful?  Maybe I am missing something.

> +  emit_insn (gen_addi_neg16 (tmp, operands[2]));
> +  emit_insn (gen_altivec_lvsl_reg (shift_mask, tmp));
> +  emit_insn (gen_lxvll (rtx_vtmp, operands[1], operands[2]));
> +  emit_insn (gen_altivec_vperm_v8hiv16qi (operands[0], rtx_vtmp,
> +					  rtx_vtmp, shift_mask));



> +;; Store VSX Vector with Length, right justified

_left_ justified?

> +(define_expand "stxvll"
> +  [(set (match_dup 3)
> +	(match_operand:DI 2 "register_operand"))
> +   (set (mem:V16QI (match_operand:DI 1 "gpc_reg_operand"))
> +	(unspec:V16QI
> +	 [(match_operand:V16QI 0 "vsx_register_operand")
> +	  (match_dup 3)]
> +	 UNSPEC_STXVLL))]
> +  "TARGET_P9_VECTOR && TARGET_64BIT"
> +{
> +  operands[3] = gen_reg_rtx (DImode);
> +})


> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtins-5-p9-runnable.c
> @@ -0,0 +1,309 @@
> +/* { dg-do run { target { powerpc64*-*-* && { p9vector_hw } } } } */

This should be powerpc*-*-* I think?  Does it need braces around
p9vector_hw?


Segher