Message ID | 20220113202033.3320854-1-philipp.tomsich@vrull.eu |
---|---|
State | New |
Headers | show |
Series | [v2,1/2] target/riscv: iterate over a table of decoders | expand |
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) >
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~
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; } } + + if (!handled) + gen_exception_illegal(ctx); } static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
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(-)