[PATCH rs6000]Fix PR92132

[Segher Boessenkool]

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