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

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

Hi Segher,

Thanks for the comments!

on 2023/2/19 20:12, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Feb 17, 2023 at 11:33:16AM +0800, Kewen.Lin wrote:
>> on 2023/2/16 23:10, Segher Boessenkool wrote:
>>> 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.  :)
> 
> Yeah.  Often, even usually, unspecs differ in some crucial ways from
> similarly named RTL expressions: you would not want an unspec at all
> otherwise!
> 
>>> 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.
> 
> No, that is not true at all.  Builtins do **not** guarantee to expand to
> any specific machine instruction.  This is one reason why such names are
> not so good, are quite misleading.

OK, I agree that we don't claim there is a 1-1 map, but for those bifs
*_(vsx|altivec)_<mnemonic>, it looks that we map them with the corresponding hw
insn (mnemonic in the name) all the time.  IMHO, it makes sense, since otherwise
it would be quite misleading (it should use one general name instead).

For this particular built-in __builtin_altivec_vprtyb[wdq], I think we all
agree that we don't want to expand it into vpopcntb + vprtyb[wdq].  :)

> 
> If you want specific machine insns, you need to use inline asm, that is
> what it is there for.  Builtins generate code with some specified
> semantics, nothing more, nothing less; just like everything else the
> compiler does, the "as-if" rule in full swing.
> 
>>>> 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
> 
> Heh, it is never easy is it?  :-)

Yeah. :)

> 
>> 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.  :)
> 
> Something longer and less confusing please.  Or maybe just with the insn
> name, that isn't a problem in the machine desription (as it is for
> builtin names or other user-facing stuff).  "rs6000_vprtyb" maybe?

Thanks for the suggestion!  Will go with "rs6000_vprtyb" if the others in
v2 [1] look good to you.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612212.html

BR,
Kewen