Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

[Robert Foley]

Hi,
On Fri, 7 Feb 2020 at 10:01, Alex Bennée <address@hidden> wrote:
> -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 = extract32(opcode, 13, 3);

We noticed that a uint16_t opcode is passed into this function and
then passed on to extract32().
This is a minor point, but the extract32() will validate the start and
length values passed in according to 32 bits, not 16 bits.
static inline uint32_t extract32(uint32_t value, int start, int length)
{
    assert(start >= 0 && length > 0 && length <= 32 - start);
Since we have an extract32() and extract64(), maybe we could add an
extract16() and use it here?

Thanks & Regards,
-Rob
> +    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 = extract32(opcode, 0, 2);
>
>      switch (op) {
>      case 0:
> -        decode_RV32_64C0(ctx);
> +        decode_RV32_64C0(ctx, opcode);
>          break;
>      }
>  }
> @@ -709,22 +708,24 @@ 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 (extract32(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 +777,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
>