On Tue, Oct 29, 2019 at 01:16:53PM +0800, Kewen.Lin wrote: > (vcond_mask_<mode><mode>): New expand.
Say for which mode please? Like (vcond_mask_<mode><mode> for VEC_I and VEC_I): New expand. > (vcond_mask_<mode><VEC_int>): Likewise. "for VEC_I and VEC_F", here, but the actual names in the pattern are for vector modes of same-size integer elements. Maybe it is clear enough like this, dunno. > (vector_{ungt,unge,unlt,unle}<mode>): Likewise. Never use wildcards (or shell expansions) in the "what changed" part of a changelog, because people try to search for that. > ;; 128-bit one's complement > -(define_insn_and_split "*one_cmpl<mode>3_internal" > +(define_insn_and_split "one_cmpl<mode>3_internal" Instead, rename it to "one_cmpl<mode>3" and delete the define_expand that serves no function? > +(define_code_iterator fpcmpun [ungt unge unlt unle]) Why these four? Should there be more? Should this be added to some existing iterator? It's not all comparisons including unordered, there are uneq, unordered itself, and ne as well. > +;; Same mode for condition true/false values and predicate operand. > +(define_expand "vcond_mask_<mode><mode>" > + [(match_operand:VEC_I 0 "vint_operand") > + (match_operand:VEC_I 1 "vint_operand") > + (match_operand:VEC_I 2 "vint_operand") > + (match_operand:VEC_I 3 "vint_operand")] > + "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)" > +{ > + emit_insn (gen_vector_select_<mode> (operands[0], operands[2], operands[1], > + operands[3])); > + DONE; > +}) So is this exactly the same as vsel/xxsel? > +;; For signed integer vectors comparison. > +(define_expand "vec_cmp<mode><mode>" > + case GEU: > + emit_insn ( > + gen_vector_nltu<mode> (operands[0], operands[2], operands[3], tmp)); > + break; > + case GTU: > + emit_insn (gen_vector_gtu<mode> (operands[0], operands[2], > operands[3])); > + break; > + case LEU: > + emit_insn ( > + gen_vector_ngtu<mode> (operands[0], operands[2], operands[3], tmp)); > + break; > + case LTU: > + emit_insn (gen_vector_gtu<mode> (operands[0], operands[3], > operands[2])); > + break; You shouldn't allow those for signed comparisons, that will only hide problems. You can do all the rest with some iterator / code attribute? Or two cases, one for the codes that need ops 2 and 3 swapped, one for the rest? > +;; For unsigned integer vectors comparison. > +(define_expand "vec_cmpu<mode><mode>" > + [(set (match_operand:VEC_I 0 "vint_operand") > + (match_operator 1 "comparison_operator" > + [(match_operand:VEC_I 2 "vint_operand") > + (match_operand:VEC_I 3 "vint_operand")]))] > + "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)" > +{ > + emit_insn (gen_vec_cmp<mode><mode> (operands[0], operands[1], > + operands[2], operands[3])); > + DONE; > +}) unsigned_comparison_operator? Why *are* there separate vec_cmp and vec_cmpu patterns, in the first place? Segher