diff mbox series

[v3,24/35] target/riscv: Move gen_arith_imm() decoding into trans_* functions

Message ID 20181031132029.4887-25-kbastian@mail.uni-paderborn.de (mailing list archive)
State New, archived
Headers show
Series target/riscv: Convert to decodetree | expand

Commit Message

Bastian Koppelmann Oct. 31, 2018, 1:20 p.m. UTC 
gen_arith_imm() does a lot of decoding manually, which was hard to read
in
case of the shift instructions and is not necessary anymore with
decodetree.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Peer Adelt <peer.adelt@hni.uni-paderborn.de>
---
v2 -> v3:
    - trans_srli/srai now use tcg_gen_shri/srai_tl
    - trans_addiw uses its own gen_addiw function which properly extends the
      result
    - &arith_imm -> &i

 target/riscv/insn32.decode              |   3 +-
 target/riscv/insn_trans/trans_rvi.inc.c |  89 +++++++++++++++-----
 target/riscv/translate.c                | 107 ++++++------------------
 3 files changed, 99 insertions(+), 100 deletions(-)

Comments

Richard Henderson Oct. 31, 2018, 10:18 p.m. UTC | #1 
On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>  static bool trans_slli(DisasContext *ctx, arg_slli *a)
>  {
> -    gen_arith_imm(ctx, OPC_RISC_SLLI, a->rd, a->rs1, a->shamt);
> +    if (a->rd != 0) {
> +        TCGv t = tcg_temp_new();
> +        gen_get_gpr(t, a->rs1);
> +
> +        if (a->shamt >= TARGET_LONG_BITS) {
> +            return false;
> +        }
> +        tcg_gen_shli_tl(t, t, a->shamt);
> +
> +        gen_set_gpr(a->rd, t);
> +        tcg_temp_free(t);
> +    } /* NOP otherwise */
>      return true;
>  }
>  
>  static bool trans_srli(DisasContext *ctx, arg_srli *a)
>  {
> -    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_I, a->rd, a->rs1, a->shamt);
> +    if (a->rd != 0) {
> +        TCGv t = tcg_temp_new();
> +        gen_get_gpr(t, a->rs1);
> +        tcg_gen_shri_tl(t, t, a->shamt);
> +        gen_set_gpr(a->rd, t);
> +        tcg_temp_free(t);
> +    } /* NOP otherwise */
>      return true;
>  }
>  
>  static bool trans_srai(DisasContext *ctx, arg_srai *a)
>  {
> -    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_I, a->rd, a->rs1, a->shamt | 0x400);
> +    if (a->rd != 0) {
> +        TCGv t = tcg_temp_new();
> +        gen_get_gpr(t, a->rs1);
> +        tcg_gen_sari_tl(t, t, a->shamt);
> +        gen_set_gpr(a->rd, t);
> +        tcg_temp_free(t);
> +    } /* NOP otherwise */
>      return true;
>  }

Surely the shri and sari functions need the same shamt >= TARGET_LONG_BITS
check as slli.  Otherwise RV32 shri should definitely produce an assert in
tcg_gen_shri_tl.

I did wonder about changing the decode of the shift functions such that only
the top two bits of the imm are reserved for secondary parsing of the shift
type, and the other 10 bits are passed down into trans_foo.  At which point the
TARGET_LONG_BITS check takes care of other illegalities.

Which means that the parsing for slli and slliw are identical, and also that
for the far future when RV128 is a thing, we don't have to change the parsing.


r~
Richard Henderson Oct. 31, 2018, 10:26 p.m. UTC | #2 
On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
> +
> +#ifdef TARGET_RISCV64
> +static void gen_addiw(TCGv ret, TCGv arg1, TCGv arg2)
> +{
> +    tcg_gen_add_tl(ret, arg1, arg2);
> +    tcg_gen_ext32s_tl(ret, ret);
> +}
> +#endif

This should have been called gen_addw.  It takes no immediate.


r~
Bastian Koppelmann Jan. 11, 2019, 1:10 p.m. UTC | #3 
On 10/31/18 11:18 PM, Richard Henderson wrote:
>
> Surely the shri and sari functions need the same shamt >= TARGET_LONG_BITS
> check as slli.  Otherwise RV32 shri should definitely produce an assert in
> tcg_gen_shri_tl.
>
> I did wonder about changing the decode of the shift functions such that only
> the top two bits of the imm are reserved for secondary parsing of the shift
> type, and the other 10 bits are passed down into trans_foo.  At which point the
> TARGET_LONG_BITS check takes care of other illegalities.
>
> Which means that the parsing for slli and slliw are identical, and also that
> for the far future when RV128 is a thing, we don't have to change the parsing.


I don't quite understand this. Do you want to have one entry in the 
decode file for slli and slliw?

How is the parsing of slli and slliw identical with this change? As far 
as I see it, they are different at least in the opcode.

Cheers,

Bastian
Richard Henderson Jan. 11, 2019, 9 p.m. UTC | #4 
On 1/12/19 12:10 AM, Bastian Koppelmann wrote:
> 
> On 10/31/18 11:18 PM, Richard Henderson wrote:
>>
>> Surely the shri and sari functions need the same shamt >= TARGET_LONG_BITS
>> check as slli.  Otherwise RV32 shri should definitely produce an assert in
>> tcg_gen_shri_tl.
>>
>> I did wonder about changing the decode of the shift functions such that only
>> the top two bits of the imm are reserved for secondary parsing of the shift
>> type, and the other 10 bits are passed down into trans_foo.  At which point the
>> TARGET_LONG_BITS check takes care of other illegalities.
>>
>> Which means that the parsing for slli and slliw are identical, and also that
>> for the far future when RV128 is a thing, we don't have to change the parsing.
> 
> 
> I don't quite understand this. Do you want to have one entry in the decode file
> for slli and slliw?
> 
> How is the parsing of slli and slliw identical with this change? As far as I
> see it, they are different at least in the opcode.

I meant in the extraction and validation of operands, I think.
I'm not really sure where else I was going with this.  It has
been two months and I don't have the decode in front of me.


r~
Bastian Koppelmann Jan. 18, 2019, noon UTC | #5 
On 1/11/19 10:00 PM, Richard Henderson wrote:
> On 1/12/19 12:10 AM, Bastian Koppelmann wrote:
>> On 10/31/18 11:18 PM, Richard Henderson wrote:
>>> Surely the shri and sari functions need the same shamt >= TARGET_LONG_BITS
>>> check as slli.  Otherwise RV32 shri should definitely produce an assert in
>>> tcg_gen_shri_tl.
>>>
>>> I did wonder about changing the decode of the shift functions such that only
>>> the top two bits of the imm are reserved for secondary parsing of the shift
>>> type, and the other 10 bits are passed down into trans_foo.  At which point the
>>> TARGET_LONG_BITS check takes care of other illegalities.
>>>
>>> Which means that the parsing for slli and slliw are identical, and also that
>>> for the far future when RV128 is a thing, we don't have to change the parsing.
>>
>> I don't quite understand this. Do you want to have one entry in the decode file
>> for slli and slliw?
>>
>> How is the parsing of slli and slliw identical with this change? As far as I
>> see it, they are different at least in the opcode.
> I meant in the extraction and validation of operands, I think.
> I'm not really sure where else I was going with this.  It has
> been two months and I don't have the decode in front of me.


Okay, I extended the immediate to 10 bits for the next patch round 
(which I send out soon) which should not break anything. I suggest we 
leave this optimization for another patch as this set is already long 
and complex.

Cheers,

Bastian
diff mbox series

Patch

diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
index 379a5d5438..70e00412b6 100644
--- a/target/riscv/insn32.decode
+++ b/target/riscv/insn32.decode
@@ -35,12 +35,13 @@ 
 
 # Argument sets:
 &b    imm rs2 rs1
+&i    imm rs1 rd
 &shift     shamt rs1 rd
 &atomic    aq rl rs2 rs1 rd
 
 # Formats 32:
 @r       .......   ..... ..... ... ..... .......                   %rs2 %rs1 %rd
-@i       ............    ..... ... ..... .......         imm=%imm_i     %rs1 %rd
+@i       ............    ..... ... ..... ....... &i      imm=%imm_i     %rs1 %rd
 @b       .......   ..... ..... ... ..... ....... &b      imm=%imm_b %rs2 %rs1
 @s       .......   ..... ..... ... ..... .......         imm=%imm_s %rs2 %rs1
 @u       ....................      ..... .......         imm=%imm_u          %rd
diff --git a/target/riscv/insn_trans/trans_rvi.inc.c b/target/riscv/insn_trans/trans_rvi.inc.c
index cf05176ba2..8bd2752f31 100644
--- a/target/riscv/insn_trans/trans_rvi.inc.c
+++ b/target/riscv/insn_trans/trans_rvi.inc.c
@@ -217,52 +217,85 @@  static bool trans_sd(DisasContext *ctx, arg_sd *a)
 
 static bool trans_addi(DisasContext *ctx, arg_addi *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_ADDI, a->rd, a->rs1, a->imm);
-    return true;
+    return gen_arith_imm(ctx, a, &tcg_gen_add_tl);
 }
 
 static bool trans_slti(DisasContext *ctx, arg_slti *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SLTI, a->rd, a->rs1, a->imm);
+    TCGv source1;
+    source1 = tcg_temp_new();
+    gen_get_gpr(source1, a->rs1);
+
+    tcg_gen_setcondi_tl(TCG_COND_LT, source1, source1, a->imm);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
     return true;
 }
 
 static bool trans_sltiu(DisasContext *ctx, arg_sltiu *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SLTIU, a->rd, a->rs1, a->imm);
+    TCGv source1;
+    source1 = tcg_temp_new();
+    gen_get_gpr(source1, a->rs1);
+
+    tcg_gen_setcondi_tl(TCG_COND_LTU, source1, source1, a->imm);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
     return true;
 }
 
 static bool trans_xori(DisasContext *ctx, arg_xori *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_XORI, a->rd, a->rs1, a->imm);
-    return true;
+    return gen_arith_imm(ctx, a, &tcg_gen_xor_tl);
 }
 static bool trans_ori(DisasContext *ctx, arg_ori *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_ORI, a->rd, a->rs1, a->imm);
-    return true;
+    return gen_arith_imm(ctx, a, &tcg_gen_or_tl);
 }
 static bool trans_andi(DisasContext *ctx, arg_andi *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_ANDI, a->rd, a->rs1, a->imm);
-    return true;
+    return gen_arith_imm(ctx, a, &tcg_gen_and_tl);
 }
 static bool trans_slli(DisasContext *ctx, arg_slli *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SLLI, a->rd, a->rs1, a->shamt);
+    if (a->rd != 0) {
+        TCGv t = tcg_temp_new();
+        gen_get_gpr(t, a->rs1);
+
+        if (a->shamt >= TARGET_LONG_BITS) {
+            return false;
+        }
+        tcg_gen_shli_tl(t, t, a->shamt);
+
+        gen_set_gpr(a->rd, t);
+        tcg_temp_free(t);
+    } /* NOP otherwise */
     return true;
 }
 
 static bool trans_srli(DisasContext *ctx, arg_srli *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_I, a->rd, a->rs1, a->shamt);
+    if (a->rd != 0) {
+        TCGv t = tcg_temp_new();
+        gen_get_gpr(t, a->rs1);
+        tcg_gen_shri_tl(t, t, a->shamt);
+        gen_set_gpr(a->rd, t);
+        tcg_temp_free(t);
+    } /* NOP otherwise */
     return true;
 }
 
 static bool trans_srai(DisasContext *ctx, arg_srai *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_I, a->rd, a->rs1, a->shamt | 0x400);
+    if (a->rd != 0) {
+        TCGv t = tcg_temp_new();
+        gen_get_gpr(t, a->rs1);
+        tcg_gen_sari_tl(t, t, a->shamt);
+        gen_set_gpr(a->rd, t);
+        tcg_temp_free(t);
+    } /* NOP otherwise */
     return true;
 }
 
@@ -329,26 +362,44 @@  static bool trans_and(DisasContext *ctx, arg_and *a)
 #ifdef TARGET_RISCV64
 static bool trans_addiw(DisasContext *ctx, arg_addiw *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_ADDIW, a->rd, a->rs1, a->imm);
-    return true;
+    return gen_arith_imm(ctx, a, &gen_addiw);
 }
 
 static bool trans_slliw(DisasContext *ctx, arg_slliw *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SLLIW, a->rd, a->rs1, a->shamt);
+    TCGv source1;
+    source1 = tcg_temp_new();
+    gen_get_gpr(source1, a->rs1);
+
+    tcg_gen_shli_tl(source1, source1, a->shamt);
+    tcg_gen_ext32s_tl(source1, source1);
+    gen_set_gpr(a->rd, source1);
+
+    tcg_temp_free(source1);
     return true;
 }
 
 static bool trans_srliw(DisasContext *ctx, arg_srliw *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_IW, a->rd, a->rs1, a->shamt);
+    TCGv t = tcg_temp_new();
+    gen_get_gpr(t, a->rs1);
+    tcg_gen_extract_tl(t, t, a->shamt, 32 - a->shamt);
+    /* sign-extend for W instructions */
+    tcg_gen_ext32s_tl(t, t);
+    gen_set_gpr(a->rd, t);
+    tcg_temp_free(t);
     return true;
 }
 
 static bool trans_sraiw(DisasContext *ctx, arg_sraiw *a)
 {
-    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_IW , a->rd, a->rs1,
-                  a->shamt | 0x400);
+    TCGv t = tcg_temp_new();
+    gen_get_gpr(t, a->rs1);
+    tcg_gen_sextract_tl(t, t, a->shamt, 32 - a->shamt);
+    /* sign-extend for W instructions */
+    tcg_gen_ext32s_tl(t, t);
+    gen_set_gpr(a->rd, t);
+    tcg_temp_free(t);
     return true;
 }
 
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0741a543aa..de60686df4 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -390,86 +390,6 @@  static void gen_arith(DisasContext *ctx, uint32_t opc, int rd, int rs1,
     tcg_temp_free(source2);
 }
 
-static void gen_arith_imm(DisasContext *ctx, uint32_t opc, int rd,
-        int rs1, target_long imm)
-{
-    TCGv source1 = tcg_temp_new();
-    int shift_len = TARGET_LONG_BITS;
-    int shift_a;
-
-    gen_get_gpr(source1, rs1);
-
-    switch (opc) {
-    case OPC_RISC_ADDI:
-#if defined(TARGET_RISCV64)
-    case OPC_RISC_ADDIW:
-#endif
-        tcg_gen_addi_tl(source1, source1, imm);
-        break;
-    case OPC_RISC_SLTI:
-        tcg_gen_setcondi_tl(TCG_COND_LT, source1, source1, imm);
-        break;
-    case OPC_RISC_SLTIU:
-        tcg_gen_setcondi_tl(TCG_COND_LTU, source1, source1, imm);
-        break;
-    case OPC_RISC_XORI:
-        tcg_gen_xori_tl(source1, source1, imm);
-        break;
-    case OPC_RISC_ORI:
-        tcg_gen_ori_tl(source1, source1, imm);
-        break;
-    case OPC_RISC_ANDI:
-        tcg_gen_andi_tl(source1, source1, imm);
-        break;
-#if defined(TARGET_RISCV64)
-    case OPC_RISC_SLLIW:
-        shift_len = 32;
-        /* FALLTHRU */
-#endif
-    case OPC_RISC_SLLI:
-        if (imm >= shift_len) {
-            goto do_illegal;
-        }
-        tcg_gen_shli_tl(source1, source1, imm);
-        break;
-#if defined(TARGET_RISCV64)
-    case OPC_RISC_SHIFT_RIGHT_IW:
-        shift_len = 32;
-        /* FALLTHRU */
-#endif
-    case OPC_RISC_SHIFT_RIGHT_I:
-        /* differentiate on IMM */
-        shift_a = imm & 0x400;
-        imm &= 0x3ff;
-        if (imm >= shift_len) {
-            goto do_illegal;
-        }
-        if (imm != 0) {
-            if (shift_a) {
-                /* SRAI[W] */
-                tcg_gen_sextract_tl(source1, source1, imm, shift_len - imm);
-            } else {
-                /* SRLI[W] */
-                tcg_gen_extract_tl(source1, source1, imm, shift_len - imm);
-            }
-            /* No further sign-extension needed for W instructions.  */
-            opc &= ~0x8;
-        }
-        break;
-    default:
-    do_illegal:
-        gen_exception_illegal(ctx);
-        return;
-    }
-
-    if (opc & 0x8) { /* sign-extend for W instructions */
-        tcg_gen_ext32s_tl(source1, source1);
-    }
-
-    gen_set_gpr(rd, source1);
-    tcg_temp_free(source1);
-}
-
 static void gen_jal(CPURISCVState *env, DisasContext *ctx, int rd,
                     target_ulong imm)
 {
@@ -697,6 +617,33 @@  static int ex_rvc_register(int reg)
 bool decode_insn32(DisasContext *ctx, uint32_t insn);
 /* Include the auto-generated decoder for 32 bit insn */
 #include "decode_insn32.inc.c"
+
+static bool gen_arith_imm(DisasContext *ctx, arg_i *a,
+                          void(*func)(TCGv, TCGv, TCGv))
+{
+    TCGv source1, source2;
+    source1 = tcg_temp_new();
+    source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    tcg_gen_movi_tl(source2, a->imm);
+
+    (*func)(source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
+    return true;
+}
+
+#ifdef TARGET_RISCV64
+static void gen_addiw(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    tcg_gen_add_tl(ret, arg1, arg2);
+    tcg_gen_ext32s_tl(ret, ret);
+}
+#endif
+
 /* Include insn module translation function */
 #include "insn_trans/trans_rvi.inc.c"
 #include "insn_trans/trans_rvm.inc.c"