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

[Alistair Francis]

On Thu, Jan 13, 2022 at 6:28 PM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> 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.

Great! Thanks for that

Alistair

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