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

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

Hi!

On Thu, Feb 16, 2023 at 08:06:02PM +0800, Kewen.Lin wrote:
> on 2023/2/16 19:14, Segher Boessenkool wrote:
> > 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.
> 
> I thought the scalar insns prty[wd] also operate on byte
> (especially on the least significant bit in each byte),
> PARITYB(yte) seems better ...

The scalar instruction does not include a "b" in the mnemonic, and it
says nothing "byte" or "bit" in the instruction name either.  The
existing name is simpler, less confusing, simply better.

> > 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.
> 
> I'll update for vector only.  Maybe it's better with UNSPEC_VPARITY*B*?
> since the mnemonic has "b"(yte).

No, you are right that the semantics are pretty much the same.  Please
just keep UNSPEC_PARITY everywhere.

> >>    const vsll __builtin_altivec_vprtybd (vsll);
> >> -    VPRTYBD parityv2di2 {}
> >> +    VPRTYBD p9v_paritybv2di2 {}
> > 
> > Why this?  Please keep the simpler names if at all possible.
> 
> The bif would like to map with the vector parity byte insns
> directly, the parity<mode>2 can't work here any more.

Ah, because it cannot use the expander here, it has to be a define_insn?
Why is that?

> The name is updated from previous *p9v_parity<mode>2 (becoming
> to a named define_insn), I noticed there are some names with
> p8v_, p9v_, meant to keep it consistent with the context.
> You want this to be simplified as parity*b*v2di2?

Without the "b".  But that would be better then, yes.  This is a great
example why p9v_ in the name is not good: most users do not care at all
what ISA version this insn first appeared in.

> > 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>
> > 
> 
> I got the same understanding initially, but as you may have noticed
> there isn't a cmpb, it seems just to be different from the name
> parity<mode>2 so put the condition as one suffix.

Yeah.  Something for a future improvement.

> >> -(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.
> 
> Got it, good point.

And we are in stage 4 so you really really do not want something that
may be a mistake, that may cause any problems :-)

> > So first do a patch that is essentially just this?
> 
> OK, will update and test it again.

Thanks!

> > Later patches can do all other things (also, not do this expand for
> > TImode at all, ho hum).
> 
> OK, I guess all the others are for next stage1. :)

Yes exactly.  And one (small, self-contained) thing per patch please.

Thanks again,


Segher