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

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Segher,

Thanks for the comments!

on 2023/2/16 23:10, Segher Boessenkool wrote:
> 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.

OK, since it has UNSPEC, I would hope the reader can realize it's
different from RTL opcode parity and mainly operating on byte.  :)

> 
>>>>    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?

No, the above statement seems to cause some misunderstanding, let me clarify:
first, the built-in functions __builtin_altivec_vprtyb[wdq] require to be
mapped to hardware insns vprtyb[wdq] directly as the functions name show.
Before this patch, the standard pattern name parity<mode>2 expands to those
insns directly (wrongly), so it's fine to use those expanders here.  After
this patch, those expands get fixed to get parity for each vector element
(vpopcntb + vprtyb*), they are not valid to be used for expanding these
built-in functions (not 1-1 map any more), so this patch fixes it with
the correct name which maps to vprtyb*.

> 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.

The name without "b" is standard pattern name, whose semantic doesn't align
with what these insns provide and we already have the matched expander with
it ("parity<mode>2"), so we can't use the name here :(.  As you felt a name
with "b" is better than "p9v_*", I'll go with "parityb" then.  :)

>>> 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.

Got it, thanks again!

BR,
Kewen