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
