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

[Philippe Mathieu-Daudé]

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.

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