Re: [PATCH] rs6000: Fix vector parity support [PR108699]

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Thu, Feb 16, 2023 at 05:23:40PM +0800, Kewen.Lin wrote:
> This patch is to fix the handling with one more pre-insn
> vpopcntb.  It also fixes an oversight having V8HI in VEC_IP,
> replaces VParity with VEC_IP, and adjusts the existing
> UNSPEC_PARITY to a more meaningful name UNSPEC_PARITYB.

Please don't do that.  UNSPEC_PARITYB is worse than UNSPEC_PARITY,
even more so for the prtyw etc. instructions.

You might want to express the vector parity insns separately, but then
*do that*, don't rename the normal stuff as well, and use a more obvious
name like UNSPEC_VPARITY please.

>    const vsll __builtin_altivec_vprtybd (vsll);
> -    VPRTYBD parityv2di2 {}
> +    VPRTYBD p9v_paritybv2di2 {}

Why this?  Please keep the simpler names if at all possible.

>  	{
>  	  emit_insn (gen_popcntbsi2 (tmp, src));
> -	  emit_insn (gen_paritysi2_cmpb (dst, tmp));
> +	  emit_insn (gen_paritybsi2 (dst, tmp));
>  	}

It is completely non-obvious what a "paritybsi2" is.  There is no such
thing as a "parityb", not for normal people anyway.  It is very
important that names give a hint of what they stand for.

The _cmpb of the existing name indicates that a cmpb insn is generated
here as well.  Has that changed>

> -(define_insn "parity<mode>2_cmpb"
> +(define_insn "parityb<mode>2"
>    [(set (match_operand:GPR 0 "gpc_reg_operand" "=r")
> -	(unspec:GPR [(match_operand:GPR 1 "gpc_reg_operand" "r")] UNSPEC_PARITY))]
> +	(unspec:GPR [(match_operand:GPR 1 "gpc_reg_operand" "r")]
> +		    UNSPEC_PARITYB))]
>    "TARGET_CMPB && TARGET_POPCNTB"
>    "prty<wd> %0,%1"
>    [(set_attr "type" "popcnt")])

Hrm, the original name was not so good apparently.  Still, please don't
change multiple independent things in one patch, it makes the patch hard
to read and understand and very hard to spot mistakes in.

> @@ -1226,7 +1225,16 @@ (define_expand "popcount<mode>2"
>  (define_expand "parity<mode>2"
>    [(set (match_operand:VEC_IP 0 "register_operand")
>  	(parity:VEC_IP (match_operand:VEC_IP 1 "register_operand")))]
> -  "TARGET_P9_VECTOR")
> +  "TARGET_P9_VECTOR"
> +{
> +  rtx op1 = gen_lowpart (V16QImode, operands[1]);
> +  rtx res = gen_reg_rtx (V16QImode);
> +  emit_insn (gen_popcountv16qi2 (res, op1));
> +  emit_insn (gen_p9v_parityb<mode>2 (operands[0],
> +				    gen_lowpart (<MODE>mode, res)));
> +
> +  DONE;
> +})

So first do a patch that is essentially just this?

Later patches can do all other things (also, not do this expand for
TImode at all, ho hum).


Segher