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

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

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.

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

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


Segher