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
