diff mbox series

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

Message ID 20200207150118.23007-5-alex.bennee@linaro.org
State New
Headers show
Series plugins/next | expand

Commit Message

Alex Bennée Feb. 7, 2020, 3:01 p.m. UTC 
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 <alex.bennee@linaro.org>

---
 target/riscv/translate.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

-- 
2.20.1

Comments

Robert Foley Feb. 7, 2020, 4:33 p.m. UTC | #1 
Hi,
On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> 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

>
Alex Bennée Feb. 7, 2020, 4:47 p.m. UTC | #2 
Robert Foley <robert.foley@linaro.org> writes:

> Hi,

> On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> 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?


Yeah - I did consider if it would be worth adding a extract16 helper.
There are a fair number of places in the code base where uint16_t is
silently promoted to a uint32_t (and a couple where it is downcast).

I guess 16 bit instruction words are common enough we should support
them in the utils.

-- 
Alex Bennée
Richard Henderson Feb. 11, 2020, 6 p.m. UTC | #3 
On 2/7/20 7:01 AM, Alex Bennée 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 <alex.bennee@linaro.org>

> ---

>  target/riscv/translate.c | 39 ++++++++++++++++++++-------------------

>  1 file changed, 20 insertions(+), 19 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>



r~
diff mbox series

Patch

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 14dc71156be..99f2bcf177c 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 = extract32(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 = 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) {