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

[Richard Henderson <richard.henderson@linaro.org>]

On 1/14/22 7:20 AM, Philipp Tomsich wrote:
> +static inline bool always_true_p(CPURISCVState *env  __attribute__((__unused__)),
> +                                 DisasContext *ctx  __attribute__((__unused__)))
> +{
> +    return true;
> +}

Drop the inline; the function will be instantiated so that it can be put in the table.
Drop the env pointer.  Everything this hook should examine must be in DisasContext.

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

static const.

> +        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)))

Never put an assignment in an if like this.

I think it would be cleaner to just do

     if (decode()) {
         return;
     }

and drop the handled variable entirely.


r~