Thanks for your review. Before posting a new patch, I want to check how to deal with the pr117722.c regression. See the details below.
> > --- a/gcc/config/riscv/autovec.md > > +++ b/gcc/config/riscv/autovec.md > > @@ -1120,12 +1120,25 @@ > > > > (define_expand "abs<mode>2" > > In the end it's a matter of taste but I'd prefer it if this were a > define_insn_and_split, with the splitting condition TARGET_ZVABD. > > > +(define_expand "<su>abd<mode>3" > > + [(set (match_operand:V_VLSI 0 "register_operand") > > + (unspec:V_VLSI > > + [(match_operand:V_VLSI 1 "register_operand") > > + (match_operand:V_VLSI 2 "register_operand")] > > + UNSPEC_VABD))] > > +;; Disabled when zvabd not enabled until PR119224 is resolved. > > + "TARGET_VECTOR && TARGET_ZVABD") > > I think the PR should be resolved by now, so I'd just use a > define_insn_and_split. With the pattern enabled we even get one more insn per > inner iteration. Back when we introduced it, the count was similark > Since it was already an expand before my patch, I didn't think about changing it. I've checked and PR119224 is indeed resolved. However, after changing from define_expand to define_insn_and_split, pr117722.c regressed. It now gets recognized as abd instead of sub + abs. now it generates: vmaxu.vv vminu.vv vsub.vv vzext.vf4 vadd.vv before: vwsubu.vv vwsubu.vv vmax.vv vwadd.wv Should I also update the test expectations for this case? I'm not sure. > > + > > +(define_insn_and_split "*abd<su><mode>3" > > + [(set (match_operand:V_VLSI 0 "register_operand" "=vr") > > + (unspec:V_VLSI > > + [(match_operand:V_VLSI 1 "register_operand" "vr") > > + (match_operand:V_VLSI 2 "register_operand" "vr")] > > + UNSPEC_VABD))] > > + "TARGET_VECTOR && (TARGET_ZVABD || can_create_pseudo_p ())" > > We also need can_create_pseudo_p for ZVABD, so just drop TARGET_ZVABD here. > dont understand why need can_create_pseudo_p for ZVABD，but ok to change it。 > > +; ======== > > +; == Widening absolute difference and accumulate > > +; ======== > > +(define_insn_and_split "*vwabdacc<su><mode>4" > > I don't think we need the '4' here, it's not a standard name. > Why do we need these patterns here at all? For combine? If so, they should > better go in autovec-opt.md. > naming updated. yes, for combine, I will move them to autovec-opt.md. > > + "TARGET_VECTOR && TARGET_ZVABD" > > I suppose TARGET_ZVABD implies TARGET_VECTOR? If not, it should :) > yes, will fix this. > > +} > > +[(set_attr "type" "viwalu")]) > > + > > +(define_insn_and_split "*vwabdacc_right<su><mode>4" > > That's needed because we don't canonicalize the plus in the presence of an > unspec? > yes, and i will add a comment. > > +(define_insn "@pred_abs<mode>" > > + [(set (match_operand:V_VLSI 0 "register_operand" "=vd, vd, vr, vr") > > + (if_then_else:V_VLSI > > + (unspec:<VM> > > + [(match_operand:<VM> 1 "vector_mask_operand" " vm, vm, Wc1, Wc1") > > + (match_operand 4 "vector_length_operand" " rK, rK, rK, rK") > > + (match_operand 5 "const_int_operand" " i, i, i, i") > > + (match_operand 6 "const_int_operand" " i, i, i, i") > > + (match_operand 7 "const_int_operand" " i, i, i, i") > > + (reg:SI VL_REGNUM) > > + (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE) > > + (abs:V_VLSI > > + (match_operand:V_VLSI 3 "register_operand" " vr, vr, vr, vr")) > > + (match_operand:V_VLSI 2 "vector_merge_operand" "vu, 0, vu, 0")))] > > + "TARGET_VECTOR && TARGET_ZVABD" > > + "vabs.v\t%0,%3%p1" > > + [(set_attr "type" "vialu") > > + (set_attr "mode" "<MODE>") > > + (set_attr "vl_op_idx" "4") > > + (set (attr "ta") (symbol_ref "riscv_vector::get_ta (operands[5])")) > > + (set (attr "ma") (symbol_ref "riscv_vector::get_ma (operands[6])")) > > + (set (attr "avl_type_idx") (const_int 7))]) > > I don't like special casing and actually we could argue for a vialu_unary type > entry, but as the bulk of the types and indexing is hopefully going to go away > anyway soon, I think we can leave this for now. > > ... > > > + [(set_attr "type" "vialu") > > + (set_attr "mode" "<MODE>") > > + (set_attr "vl_op_idx" "5") > > + (set (attr "ta") (symbol_ref "riscv_vector::get_ta (operands[6])")) > > + (set (attr "ma") (symbol_ref "riscv_vector::get_ma (operands[7])")) > > + (set (attr "avl_type_idx") (const_int 8))]) > > Do we need the last four attributes here, i.e. don't they match vialu's > attributes? > > ... > > See above for viwalu. Understood. They are redundant, i will remove them. > > +(define_insn "@pred_vwabdacc<su><mode>" > > This should probably be called pred_widen_... if we follow the naming from the > rest of the file. naming updated. -- Zhongyao
