Re: [PATCH v2 1/2] target/riscv: iterate over a table of decoders

[Philipp Tomsich]

On Wed, 19 Jan 2022 at 12:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 13/1/22 21:20, Philipp Tomsich wrote:
> > To split up the decoder into multiple functions (both to support
> > vendor-specific opcodes in separate files and to simplify maintenance
> > of orthogonal extensions), this changes decode_op to iterate over a
> > table of decoders predicated on guard functions.
> >
> > This commit only adds the new structure and the table, allowing for
> > the easy addition of additional decoders in the future.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> > Changes in v2:
> > - (new patch) iterate over a table of guarded decoder functions
> >
> >   target/riscv/translate.c | 38 ++++++++++++++++++++++++++++++++------
> >   1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 615048ec87..2cbf9cbb6f 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -116,6 +116,12 @@ static inline bool has_ext(DisasContext *ctx, uint32_t 
> > ext)
> >       return ctx->misa_ext & ext;
> >   }
> >
> > +static inline bool always_true_p(CPURISCVState *env  
> > __attribute__((__unused__)),
> > +                                 DisasContext *ctx  
> > __attribute__((__unused__)))
> > +{
> > +    return true;
> > +}
> > +
> >   #ifdef TARGET_RISCV32
> >   #define get_xl(ctx)    MXL_RV32
> >   #elif defined(CONFIG_USER_ONLY)
> > @@ -844,16 +850,28 @@ static uint32_t opcode_at(DisasContextBase *dcbase, 
> > target_ulong pc)
> >
> >   static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t 
> > opcode)
> >   {
> > -    /* check for compressed insn */
> > +    /* If not handled, we'll raise an illegal instruction exception */
> > +    bool handled = false;
> > +
> > +    /*
> > +     * A table with predicate (i.e., guard) functions and decoder functions
> > +     * that are tested in-order until a decoder matches onto the opcode.
> > +     */
> > +    const struct {
> > +        bool (*guard_func)(CPURISCVState *, DisasContext *);
> > +        bool (*decode_func)(DisasContext *, uint32_t);
> > +    } decoders[] = {
> > +        { always_true_p,  decode_insn32 },
> > +    };
> > +
> > +    /* Check for compressed insn */
> >       if (extract16(opcode, 0, 2) != 3) {
> >           if (!has_ext(ctx, RVC)) {
> >               gen_exception_illegal(ctx);
> >           } else {
> >               ctx->opcode = opcode;
> >               ctx->pc_succ_insn = ctx->base.pc_next + 2;
> > -            if (!decode_insn16(ctx, opcode)) {
> > -                gen_exception_illegal(ctx);
> > -            }
> > +            handled = decode_insn16(ctx, opcode);
> >           }
> >       } else {
> >           uint32_t opcode32 = opcode;
> > @@ -862,10 +880,18 @@ static void decode_opc(CPURISCVState *env, 
> > DisasContext *ctx, uint16_t opcode)
> >                                                ctx->base.pc_next + 2));
> >           ctx->opcode = opcode32;
> >           ctx->pc_succ_insn = ctx->base.pc_next + 4;
> > -        if (!decode_insn32(ctx, opcode32)) {
> > -            gen_exception_illegal(ctx);
> > +
> > +        for (size_t i = 0; i < ARRAY_SIZE(decoders); ++i) {
> > +            if (!decoders[i].guard_func(env, ctx))
> > +                continue;
> > +
> > +            if ((handled = decoders[i].decode_func(ctx, opcode32)))
> > +                break;
>
> Again, while we might check whether "Vendor Extensions" are enabled or
> not at runtime, they are specific to a (vendor) core model, so we know
> their availability  at instantiation time.
>
> I don't understand the need to iterate. You can check for vendor
> extensions in riscv_tr_init_disas_context() and set a vendor_decoder()
> handler in DisasContext, which ends calling the generic decode_opc()
> one.

While the design you propose is a valid variation that will achieve
most of the functionality, I don't believe that this is the best way
forward.
A key issue is that it will interfere with using the command-line to
enable/disable such vendor-defined extensions easily (i.e., "-cpu
any,XVentanaCondOps=true" will not work).

It also looks like there is a misunderstanding of how vendor-defined
extensions work: these will not be the same for every vendor core and
may be implemented by multiple vendors (after all: these are
vendor-defined, not vendor-specific). Trying to force the RISC-V
vendors down the route of handling this via a specialized decoder
function set up in riscv_tr_init_disas_context(), will eventually
force them to have multiple decode functions for
chip-families/generations — this is not conducive to easy
maintainability of the codebase.

Regards,
Philipp.

>
> >           }
> >       }
> > +
> > +    if (!handled)
> > +        gen_exception_illegal(ctx);
> >   }
> >
> >   static void riscv_tr_init_disas_context(DisasContextBase *dcbase, 
> > CPUState *cs)
>