Re: [PATCH v1 1/2] decodetree: Add an optional predicate-function for decoding

[Philipp Tomsich]

On Thu, 13 Jan 2022 at 06:07, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Jan 13, 2022 at 1:42 AM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > Alistair,
> >
> > Do you (and the other RISC-V custodians of target/riscv) have any opinion 
> > on this?
> > We can go either way — and it boils down to a style and process question.
>
> Sorry, it's a busy week!
>
> I had a quick look over this series and left some comments below.


Thank you for taking the time despite the busy week — I can absolutely
relate, as it seems that January is picking up right where December
left off ;-)

>
> >
> > Thanks,
> > Philipp.
> >
> > On Mon, 10 Jan 2022 at 12:30, Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > wrote:
> >>
> >> On 1/10/22 12:11, Philipp Tomsich wrote:
> >> > On Mon, 10 Jan 2022 at 11:03, Philippe Mathieu-Daudé <f4bug@amsat.org
> >> > <mailto:f4bug@amsat.org>> wrote:
> >> >
> >> >     On 1/10/22 10:52, Philipp Tomsich wrote:
> >> >     > For RISC-V the opcode decode will change between different vendor
> >> >     > implementations of RISC-V (emulated by the same qemu binary).
> >> >     > Any two vendors may reuse the same opcode space, e.g., we may end
> >> >     up with:
> >> >     >
> >> >     > # *** RV64 Custom-3 Extension ***
> >> >     > {
> >> >     >   vt_maskc   0000000  ..... ..... 110 ..... 1111011 @r
> >> >     |has_xventanacondops_p
> >> >     >   vt_maskcn  0000000  ..... ..... 111 ..... 1111011 @r
> >> >     |has_xventanacondops_p
> >> >     >   someone_something  ............ ..... 000 ..... 1100111 @i
> >> >     > |has_xsomeonesomething_p
> >> >     > }
>
> I don't love this. If even a few vendors use this we could have a huge
> number of instructions here
>
> >> >     >
> >> >     > With extensions being enabled either from the commandline
> >> >     >     -cpu any,xventanacondops=true
> >> >     > or possibly even having a AMP in one emulation setup (e.g. 
> >> > application
> >> >     > cores having one extension and power-mangement cores having a
> >> >     > different one — or even a conflicting one).
>
> Agreed, an AMP configuration is entirely possible.
>
> >> >
> >> >     I understand, I think this is what MIPS does, see commit 9d005392390:
> >> >     ("target/mips: Introduce decodetree structure for NEC Vr54xx 
> >> > extension")
> >> >
> >> >
> >> > The MIPS implementation is functionally equivalent, and I could see us
> >> > doing something similar for RISC-V (although I would strongly prefer to
> >> > make everything explicit via the .decode-file instead of relying on
> >> > people being aware of the logic in decode_op).
> >> >
> >> > With the growing number of optional extensions (as of today, at least
> >> > the Zb[abcs] and vector comes to mind), we would end up with a large
> >> > number of decode-files that will then need to be sequentially called
> >> > from decode_op(). The predicates can then move up into decode_op,
> >> > predicting the auto-generated decoders from being invoked.
> >> >
> >> > As of today, we have predicates for at least the following:
> >> >
> >> >   * Zb[abcs]
> >> >   * Vectors
>
> I see your point, having a long list of decode_*() functions to call
> is a hassle. On the other hand having thousands of lines in
> insn32.decode is also a pain.
>
> In saying that, having official RISC-V extensions in insn32.decode and
> vendor instructions in insn<vendor>.decode seems like a reasonable
> compromise. Maybe even large extensions (vector maybe?) could have
> their own insn<extension>.decode file, on a case by case basis.
>
> >> >
> >> > As long as we are in greenfield territory (i.e. not dealing with
> >> > HINT-instructions that overlap existing opcode space), this will be fine
> >> > and provide proper isolation between the .decode-tables.
> >> > However, as soon as we need to implement something along the lines (I
> >> > know this is a bad example, as prefetching will be a no-op on qemu) of:
> >> >
> >> >     {
> >> >       {
> >> >         # *** RV32 Zicbop Sandard Extension (hints in the ori-space) ***
> >> >         prefetch_i  ....... 00000 ..... 110 00000 0010011 @cbo_pref
> >> >         prefetch_r  ....... 00001 ..... 110 00000 0010011 @cbo_pref
> >> >         prefetch_w  ....... 00011 ..... 110 00000 0010011 @cbo_pref
> >> >       }
> >> >       ori      ............     ..... 110 ..... 0010011 @i
> >> >     }
> >> >
> >> > we'd need to make sure that the generated decoders are called in the
> >> > appropriate order (i.e. the decoder for the specialized instructions
> >> > will need to be called first), which would not be apparent from looking
> >> > at the individual .decode files.
> >> >
> >> > Let me know what direction we want to take (of course, I have a bias
> >> > towards the one in the patch).
> >>
> >> I can't say for RISCV performance, I am myself biased toward maintenance
> >> where having one compilation unit per (vendor) extension.
> >> ARM and MIPS seems to go in this direction, while PPC and RISCV in the
> >> other one.
>
> I think we could do both right?
>
> We could add the predicate support, but also isolate vendors to their
> own decode file
>
> So for example, for vendor abc
>
> +++ b/target/riscv/insnabc.decode
> +# *** Custom abc Extension ***
> +{
> +  vt_maskc   0000000  ..... ..... 110 ..... 1111011 @r |has_abc_c
> +  vt_maskcn  0000000  ..... ..... 111 ..... 1111011 @r |has_abc_d
> +}
>
> Then there is a decode_abc(), but we support extension abc_c and abc_d
>
> That way we can keep all the vendor code seperate, while still
> allowing flexibility. Will that work?

I'll split this up into multiple series then:
1. XVentanaCondOps will use a separate decoder (so no decodetree changes in v2).
2. A new series that adds predicate support and uses it for Zb[abcs]
3. A third series that factors vector out of the insn32.decode and
puts it into its own decoder.

This will give us the chance to discuss individual design changes at
their own speed.

Philipp.

>
> We can also then use predicate support for the standard RISC-V
> extensions as described by Philipp
>
> Alistair