Hi Carl,
On Thu, Sep 14, 2017 at 02:23:47PM -0700, Carl Love wrote:
> 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).
It is a bit more complicated on older cores I think; but we'll deal with
all at once, there is nothing special about your added one.
> * doc/extend.texi: Update the built-in documenation file for the new
> built-in functions.
(Typo, "documentation").
> +(define_insn "altivec_lvsl_reg"
> + [(set (match_operand:V16QI 0 "vsx_register_operand" "=v")
altivec_register_operand instead? lvsl can target only the VR regs, not
all VSR regs.
> +;; 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);
> +})
I don't think you need to copy operands[2] to a temporary here, see below.
Why does this require TARGET_64BIT?
> +(define_insn "sldi"
> + [(set (match_operand:DI 0 "vsx_register_operand" "=r")
> + (unspec:DI [(match_operand:DI 1 "gpc_reg_operand" "r")
> + (match_operand:DI 2 "u6bit_cint_operand" "")]
> + UNSPEC_SLDI))]
> + ""
> + "sldi %0,%1,%2"
> +)
As we discussed, you can just use ashldi3.
> +(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;"
> + [(set_attr "length" "4")
> + (set_attr "type" "vecload")])
Why "+r"? The instruction doesn't write to that reg. A leftover from
an earlier version of the patch, I guess.
No ";" at the end of pattern strings please.
Length 4 is the default, just leave it out.
> +;; 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"
Expanders don't need constraints; just leave them out :-)
> +{
> + 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: operands[2] is between 0 and 15, adding -16 to it results
> + in a negative value. Shifting left by a negative value results in
> + the value being shifted right by the desired amount. */
> + emit_insn (gen_adddi3 (tmp, operands[2], GEN_INT (-16)));
> + emit_insn (gen_altivec_lvsl_reg (shift_mask, tmp));
Since lvsl looks only at the low four bits, adding -16 does nothing for it.
> + emit_insn (gen_sldi (operands[2], operands[2], GEN_INT (56)));
Please use a new temporary instead of reusing operands[2]; this gives the
register allocator more freedom.
> +(define_insn "*stxvll"
> + [(set (mem:V16QI (match_operand:DI 1 "gpc_reg_operand" "b"))
> + (unspec:V16QI
> + [(match_operand:V16QI 0 "vsx_register_operand" "wa")
> + (match_operand:DI 2 "register_operand" "+r")]
> + UNSPEC_STXVLL))]
> + "TARGET_P9_VECTOR && TARGET_64BIT"
> + "stxvll %x0,%1,%2"
> + [(set_attr "length" "8")
> + (set_attr "type" "vecstore")])
That's the wrong length now (just a single insn; doesn't need a length
attribute).
Many of these comments apply to multiple places, please check all.
Thanks,
Segher
