Re: [PATCH v2 14/19] target/riscv: progressively load the instruction during decode

[Alistair Francis]

On Thu, Feb 13, 2020 at 3:08 PM Alex Bennée <address@hidden> wrote:
>
> The plugin system would throw up a harmless warning when it detected
> that a disassembly of an instruction didn't use all it's bytes. Fix
> the riscv decoder to only load the instruction bytes it needs as it
> needs them.
>
> This drops opcode from the ctx in favour if passing the appropriately
> sized opcode down a few levels of the decode.
>
> Signed-off-by: Alex Bennée <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

>
> ---
> v2
>   - use extract16 for uint16_t opcodes
>
> squash! target/riscv: progressively load the instruction during decode
> ---
>  target/riscv/instmap.h   |  8 ++++----
>  target/riscv/translate.c | 40 +++++++++++++++++++++-------------------
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/target/riscv/instmap.h b/target/riscv/instmap.h
> index f8ad7d60fd5..40b6d2b64de 100644
> --- a/target/riscv/instmap.h
> +++ b/target/riscv/instmap.h
> @@ -344,8 +344,8 @@ enum {
>  #define GET_C_LW_IMM(inst)          ((extract32(inst, 6, 1) << 2) \
>                                      | (extract32(inst, 10, 3) << 3) \
>                                      | (extract32(inst, 5, 1) << 6))
> -#define GET_C_LD_IMM(inst)          ((extract32(inst, 10, 3) << 3) \
> -                                    | (extract32(inst, 5, 2) << 6))
> +#define GET_C_LD_IMM(inst)          ((extract16(inst, 10, 3) << 3) \
> +                                    | (extract16(inst, 5, 2) << 6))
>  #define GET_C_J_IMM(inst)           ((extract32(inst, 3, 3) << 1) \
>                                      | (extract32(inst, 11, 1) << 4) \
>                                      | (extract32(inst, 2, 1) << 5) \
> @@ -363,7 +363,7 @@ enum {
>  #define GET_C_RD(inst)              GET_RD(inst)
>  #define GET_C_RS1(inst)             GET_RD(inst)
>  #define GET_C_RS2(inst)             extract32(inst, 2, 5)
> -#define GET_C_RS1S(inst)            (8 + extract32(inst, 7, 3))
> -#define GET_C_RS2S(inst)            (8 + extract32(inst, 2, 3))
> +#define GET_C_RS1S(inst)            (8 + extract16(inst, 7, 3))
> +#define GET_C_RS2S(inst)            (8 + extract16(inst, 2, 3))
>
>  #endif
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 14dc71156be..d5de7f468a7 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -44,7 +44,6 @@ typedef struct DisasContext {
>      /* pc_succ_insn points to the instruction following base.pc_next */
>      target_ulong pc_succ_insn;
>      target_ulong priv_ver;
> -    uint32_t opcode;
>      uint32_t mstatus_fs;
>      uint32_t misa;
>      uint32_t mem_idx;
> @@ -492,45 +491,45 @@ static void gen_set_rm(DisasContext *ctx, int rm)
>      tcg_temp_free_i32(t0);
>  }
>
> -static void decode_RV32_64C0(DisasContext *ctx)
> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
> +    uint8_t funct3 = extract16(opcode, 13, 3);
> +    uint8_t rd_rs2 = GET_C_RS2S(opcode);
> +    uint8_t rs1s = GET_C_RS1S(opcode);
>
>      switch (funct3) {
>      case 3:
>  #if defined(TARGET_RISCV64)
>          /* C.LD(RV64/128) -> ld rd', offset[7:3](rs1')*/
>          gen_load_c(ctx, OPC_RISC_LD, rd_rs2, rs1s,
> -                 GET_C_LD_IMM(ctx->opcode));
> +                 GET_C_LD_IMM(opcode));
>  #else
>          /* C.FLW (RV32) -> flw rd', offset[6:2](rs1')*/
>          gen_fp_load(ctx, OPC_RISC_FLW, rd_rs2, rs1s,
> -                    GET_C_LW_IMM(ctx->opcode));
> +                    GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      case 7:
>  #if defined(TARGET_RISCV64)
>          /* C.SD (RV64/128) -> sd rs2', offset[7:3](rs1')*/
>          gen_store_c(ctx, OPC_RISC_SD, rs1s, rd_rs2,
> -                  GET_C_LD_IMM(ctx->opcode));
> +                  GET_C_LD_IMM(opcode));
>  #else
>          /* C.FSW (RV32) -> fsw rs2', offset[6:2](rs1')*/
>          gen_fp_store(ctx, OPC_RISC_FSW, rs1s, rd_rs2,
> -                     GET_C_LW_IMM(ctx->opcode));
> +                     GET_C_LW_IMM(opcode));
>  #endif
>          break;
>      }
>  }
>
> -static void decode_RV32_64C(DisasContext *ctx)
> +static void decode_RV32_64C(DisasContext *ctx, uint16_t opcode)
>  {
> -    uint8_t op = extract32(ctx->opcode, 0, 2);
> +    uint8_t op = extract16(opcode, 0, 2);
>
>      switch (op) {
>      case 0:
> -        decode_RV32_64C0(ctx);
> +        decode_RV32_64C0(ctx, opcode);
>          break;
>      }
>  }
> @@ -709,22 +708,25 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
>  /* Include the auto-generated decoder for 16 bit insn */
>  #include "decode_insn16.inc.c"
>
> -static void decode_opc(DisasContext *ctx)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t 
> opcode)
>  {
>      /* check for compressed insn */
> -    if (extract32(ctx->opcode, 0, 2) != 3) {
> +    if (extract16(opcode, 0, 2) != 3) {
>          if (!has_ext(ctx, RVC)) {
>              gen_exception_illegal(ctx);
>          } else {
>              ctx->pc_succ_insn = ctx->base.pc_next + 2;
> -            if (!decode_insn16(ctx, ctx->opcode)) {
> +            if (!decode_insn16(ctx, opcode)) {
>                  /* fall back to old decoder */
> -                decode_RV32_64C(ctx);
> +                decode_RV32_64C(ctx, opcode);
>              }
>          }
>      } else {
> +        uint32_t opcode32 = opcode;
> +        opcode32 = deposit32(opcode32, 16, 16,
> +                             translator_lduw(env, ctx->base.pc_next + 2));
>          ctx->pc_succ_insn = ctx->base.pc_next + 4;
> -        if (!decode_insn32(ctx, ctx->opcode)) {
> +        if (!decode_insn32(ctx, opcode32)) {
>              gen_exception_illegal(ctx);
>          }
>      }
> @@ -776,9 +778,9 @@ static void riscv_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPURISCVState *env = cpu->env_ptr;
> +    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
>
> -    ctx->opcode = translator_ldl(env, ctx->base.pc_next);
> -    decode_opc(ctx);
> +    decode_opc(env, ctx, opcode16);
>      ctx->base.pc_next = ctx->pc_succ_insn;
>
>      if (ctx->base.is_jmp == DISAS_NEXT) {
> --
> 2.20.1
>
>