Hi Segher,
Thanks for the review comments!
on 2023/2/16 19:14, Segher Boessenkool wrote:
> 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.
I thought the scalar insns prty[wd] also operate on byte
(especially on the least significant bit in each byte),
PARITYB(yte) seems 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).
>
>> 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.
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?
>
>> {
>> 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>
>
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.
>> -(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.
>
>> @@ -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?
OK, will update and test it again.
>
> 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. :)
BR,
Kewen
