diff mbox series

[v3,26/35] target/riscv: Remove shift and slt insn manual decoding

Message ID 20181031132029.4887-27-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 
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Signed-off-by: Peer Adelt <peer.adelt@hni.uni-paderborn.de>
---
 target/riscv/insn_trans/trans_rvi.inc.c | 78 +++++++++++++++++++++----
 target/riscv/translate.c                | 59 ++++++-------------
 2 files changed, 85 insertions(+), 52 deletions(-)

Comments

Richard Henderson Oct. 31, 2018, 10:38 p.m. UTC | #1 
On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>  static bool trans_slt(DisasContext *ctx, arg_slt *a)
>  {
> -    gen_arith(ctx, OPC_RISC_SLT, a->rd, a->rs1, a->rs2);
> +    TCGv source1 = tcg_temp_new();
> +    TCGv source2 = tcg_temp_new();
> +
> +    gen_get_gpr(source1, a->rs1);
> +    gen_get_gpr(source2, a->rs2);
> +
> +    tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);

I do wonder about extracting this one line to gen_slt so that you can re-use
gen_arith and gen_arithi.

> +    tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);

Similarly.

>  static bool trans_sllw(DisasContext *ctx, arg_sllw *a)
>  {
> -    gen_arith(ctx, OPC_RISC_SLLW, a->rd, a->rs1, a->rs2);
> +    TCGv source1 = tcg_temp_new();
> +    TCGv source2 = tcg_temp_new();
> +
> +    gen_get_gpr(source1, a->rs1);
> +    gen_get_gpr(source2, a->rs2);
> +
> +    tcg_gen_andi_tl(source2, source2, 0x1F);
> +    tcg_gen_shl_tl(source1, source1, source2);
> +
> +    gen_set_gpr(a->rd, source1);

Missing the ext32s after the shift.

>  static bool trans_srlw(DisasContext *ctx, arg_srlw *a)
>  {
> -    gen_arith(ctx, OPC_RISC_SRLW, a->rd, a->rs1, a->rs2);
> +    TCGv source1 = tcg_temp_new();
> +    TCGv source2 = tcg_temp_new();
> +
> +    gen_get_gpr(source1, a->rs1);
> +    gen_get_gpr(source2, a->rs2);
> +
> +    /* clear upper 32 */
> +    tcg_gen_ext32u_tl(source1, source1);
> +    tcg_gen_andi_tl(source2, source2, 0x1F);
> +    tcg_gen_shr_tl(source1, source1, source2);

Likewise.  (Consider source2 == 0.)

> -    case OPC_RISC_SRL:
> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
> -        tcg_gen_shr_tl(source1, source1, source2);
> -        break;
...
> -    case OPC_RISC_SRA:
> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
> -        tcg_gen_sar_tl(source1, source1, source2);
> -        break;

I see that the bugs are in the original though, so fixing them in a separate
patch is certainly ok.


r~
Palmer Dabbelt Nov. 1, 2018, 3:59 p.m. UTC | #2 
On Wed, 31 Oct 2018 15:38:08 PDT (-0700), richard.henderson@linaro.org wrote:
> On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>>  static bool trans_slt(DisasContext *ctx, arg_slt *a)
>>  {
>> -    gen_arith(ctx, OPC_RISC_SLT, a->rd, a->rs1, a->rs2);
>> +    TCGv source1 = tcg_temp_new();
>> +    TCGv source2 = tcg_temp_new();
>> +
>> +    gen_get_gpr(source1, a->rs1);
>> +    gen_get_gpr(source2, a->rs2);
>> +
>> +    tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);
>
> I do wonder about extracting this one line to gen_slt so that you can re-use
> gen_arith and gen_arithi.
>
>> +    tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);
>
> Similarly.
>
>>  static bool trans_sllw(DisasContext *ctx, arg_sllw *a)
>>  {
>> -    gen_arith(ctx, OPC_RISC_SLLW, a->rd, a->rs1, a->rs2);
>> +    TCGv source1 = tcg_temp_new();
>> +    TCGv source2 = tcg_temp_new();
>> +
>> +    gen_get_gpr(source1, a->rs1);
>> +    gen_get_gpr(source2, a->rs2);
>> +
>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>> +    tcg_gen_shl_tl(source1, source1, source2);
>> +
>> +    gen_set_gpr(a->rd, source1);
>
> Missing the ext32s after the shift.
>
>>  static bool trans_srlw(DisasContext *ctx, arg_srlw *a)
>>  {
>> -    gen_arith(ctx, OPC_RISC_SRLW, a->rd, a->rs1, a->rs2);
>> +    TCGv source1 = tcg_temp_new();
>> +    TCGv source2 = tcg_temp_new();
>> +
>> +    gen_get_gpr(source1, a->rs1);
>> +    gen_get_gpr(source2, a->rs2);
>> +
>> +    /* clear upper 32 */
>> +    tcg_gen_ext32u_tl(source1, source1);
>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>> +    tcg_gen_shr_tl(source1, source1, source2);
>
> Likewise.  (Consider source2 == 0.)
>
>> -    case OPC_RISC_SRL:
>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>> -        tcg_gen_shr_tl(source1, source1, source2);
>> -        break;
> ...
>> -    case OPC_RISC_SRA:
>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>> -        tcg_gen_sar_tl(source1, source1, source2);
>> -        break;
>
> I see that the bugs are in the original though, so fixing them in a separate
> patch is certainly ok.

Since we're in the soft freeze now, I think it would be great to get the bug 
fixes broken out as separate patches at the start of this series that we can 
pick up for this release.  It would be great if the decodetree conversion was a 
non-functional change, as that will make it easier to review.

Sorry for adding a bunch of work for you, Bastian :)
Bastian Koppelmann Nov. 5, 2018, 5 p.m. UTC | #3 
On 11/1/18 4:59 PM, Palmer Dabbelt wrote:
> On Wed, 31 Oct 2018 15:38:08 PDT (-0700), richard.henderson@linaro.org 
> wrote:
>> On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>>>  static bool trans_slt(DisasContext *ctx, arg_slt *a)
>>>  {
>>> -    gen_arith(ctx, OPC_RISC_SLT, a->rd, a->rs1, a->rs2);
>>> +    TCGv source1 = tcg_temp_new();
>>> +    TCGv source2 = tcg_temp_new();
>>> +
>>> +    gen_get_gpr(source1, a->rs1);
>>> +    gen_get_gpr(source2, a->rs2);
>>> +
>>> +    tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);
>>
>> I do wonder about extracting this one line to gen_slt so that you can 
>> re-use
>> gen_arith and gen_arithi.
>>
>>> +    tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);
>>
>> Similarly.
>>
>>>  static bool trans_sllw(DisasContext *ctx, arg_sllw *a)
>>>  {
>>> -    gen_arith(ctx, OPC_RISC_SLLW, a->rd, a->rs1, a->rs2);
>>> +    TCGv source1 = tcg_temp_new();
>>> +    TCGv source2 = tcg_temp_new();
>>> +
>>> +    gen_get_gpr(source1, a->rs1);
>>> +    gen_get_gpr(source2, a->rs2);
>>> +
>>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>>> +    tcg_gen_shl_tl(source1, source1, source2);
>>> +
>>> +    gen_set_gpr(a->rd, source1);
>>
>> Missing the ext32s after the shift.
>>
>>>  static bool trans_srlw(DisasContext *ctx, arg_srlw *a)
>>>  {
>>> -    gen_arith(ctx, OPC_RISC_SRLW, a->rd, a->rs1, a->rs2);
>>> +    TCGv source1 = tcg_temp_new();
>>> +    TCGv source2 = tcg_temp_new();
>>> +
>>> +    gen_get_gpr(source1, a->rs1);
>>> +    gen_get_gpr(source2, a->rs2);
>>> +
>>> +    /* clear upper 32 */
>>> +    tcg_gen_ext32u_tl(source1, source1);
>>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>>> +    tcg_gen_shr_tl(source1, source1, source2);
>>
>> Likewise.  (Consider source2 == 0.)
>>
>>> -    case OPC_RISC_SRL:
>>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>>> -        tcg_gen_shr_tl(source1, source1, source2);
>>> -        break;
>> ...
>>> -    case OPC_RISC_SRA:
>>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>>> -        tcg_gen_sar_tl(source1, source1, source2);
>>> -        break;
>>
>> I see that the bugs are in the original though, so fixing them in a 
>> separate
>> patch is certainly ok.
>
> Since we're in the soft freeze now, I think it would be great to get 
> the bug fixes broken out as separate patches at the start of this 
> series that we can pick up for this release.  It would be great if the 
> decodetree conversion was a non-functional change, as that will make 
> it easier to review.


Sounds good. Unfortunately, I don't have much time right now. I try at 
least to extract the bugfixes, such that they make it for this release.


Cheers,

Bastian
Palmer Dabbelt Nov. 7, 2018, 12:56 a.m. UTC | #4 
On Mon, 05 Nov 2018 09:00:10 PST (-0800), Bastian Koppelmann wrote:
>
> On 11/1/18 4:59 PM, Palmer Dabbelt wrote:
>> On Wed, 31 Oct 2018 15:38:08 PDT (-0700), richard.henderson@linaro.org
>> wrote:
>>> On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>>>>  static bool trans_slt(DisasContext *ctx, arg_slt *a)
>>>>  {
>>>> -    gen_arith(ctx, OPC_RISC_SLT, a->rd, a->rs1, a->rs2);
>>>> +    TCGv source1 = tcg_temp_new();
>>>> +    TCGv source2 = tcg_temp_new();
>>>> +
>>>> +    gen_get_gpr(source1, a->rs1);
>>>> +    gen_get_gpr(source2, a->rs2);
>>>> +
>>>> +    tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);
>>>
>>> I do wonder about extracting this one line to gen_slt so that you can
>>> re-use
>>> gen_arith and gen_arithi.
>>>
>>>> +    tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);
>>>
>>> Similarly.
>>>
>>>>  static bool trans_sllw(DisasContext *ctx, arg_sllw *a)
>>>>  {
>>>> -    gen_arith(ctx, OPC_RISC_SLLW, a->rd, a->rs1, a->rs2);
>>>> +    TCGv source1 = tcg_temp_new();
>>>> +    TCGv source2 = tcg_temp_new();
>>>> +
>>>> +    gen_get_gpr(source1, a->rs1);
>>>> +    gen_get_gpr(source2, a->rs2);
>>>> +
>>>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>>>> +    tcg_gen_shl_tl(source1, source1, source2);
>>>> +
>>>> +    gen_set_gpr(a->rd, source1);
>>>
>>> Missing the ext32s after the shift.
>>>
>>>>  static bool trans_srlw(DisasContext *ctx, arg_srlw *a)
>>>>  {
>>>> -    gen_arith(ctx, OPC_RISC_SRLW, a->rd, a->rs1, a->rs2);
>>>> +    TCGv source1 = tcg_temp_new();
>>>> +    TCGv source2 = tcg_temp_new();
>>>> +
>>>> +    gen_get_gpr(source1, a->rs1);
>>>> +    gen_get_gpr(source2, a->rs2);
>>>> +
>>>> +    /* clear upper 32 */
>>>> +    tcg_gen_ext32u_tl(source1, source1);
>>>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>>>> +    tcg_gen_shr_tl(source1, source1, source2);
>>>
>>> Likewise.  (Consider source2 == 0.)
>>>
>>>> -    case OPC_RISC_SRL:
>>>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>>>> -        tcg_gen_shr_tl(source1, source1, source2);
>>>> -        break;
>>> ...
>>>> -    case OPC_RISC_SRA:
>>>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>>>> -        tcg_gen_sar_tl(source1, source1, source2);
>>>> -        break;
>>>
>>> I see that the bugs are in the original though, so fixing them in a
>>> separate
>>> patch is certainly ok.
>>
>> Since we're in the soft freeze now, I think it would be great to get
>> the bug fixes broken out as separate patches at the start of this
>> series that we can pick up for this release.  It would be great if the
>> decodetree conversion was a non-functional change, as that will make
>> it easier to review.
>
>
> Sounds good. Unfortunately, I don't have much time right now. I try at
> least to extract the bugfixes, such that they make it for this release.

Thanks.  You're welcome to just enumerate the bug fixes so I can dig them out 
if that's easier.
diff mbox series

Patch

diff --git a/target/riscv/insn_trans/trans_rvi.inc.c b/target/riscv/insn_trans/trans_rvi.inc.c
index 5b5999954a..65548d1281 100644
--- a/target/riscv/insn_trans/trans_rvi.inc.c
+++ b/target/riscv/insn_trans/trans_rvi.inc.c
@@ -311,19 +311,38 @@  static bool trans_sub(DisasContext *ctx, arg_sub *a)
 
 static bool trans_sll(DisasContext *ctx, arg_sll *a)
 {
-    gen_arith(ctx, OPC_RISC_SLL, a->rd, a->rs1, a->rs2);
-    return true;
+    return gen_shift(ctx, a, &tcg_gen_shl_tl);
 }
 
 static bool trans_slt(DisasContext *ctx, arg_slt *a)
 {
-    gen_arith(ctx, OPC_RISC_SLT, a->rd, a->rs1, a->rs2);
+    TCGv source1 = tcg_temp_new();
+    TCGv source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    gen_get_gpr(source2, a->rs2);
+
+    tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
     return true;
 }
 
 static bool trans_sltu(DisasContext *ctx, arg_sltu *a)
 {
-    gen_arith(ctx, OPC_RISC_SLTU, a->rd, a->rs1, a->rs2);
+    TCGv source1 = tcg_temp_new();
+    TCGv source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    gen_get_gpr(source2, a->rs2);
+
+    tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
     return true;
 }
 
@@ -334,14 +353,12 @@  static bool trans_xor(DisasContext *ctx, arg_xor *a)
 
 static bool trans_srl(DisasContext *ctx, arg_srl *a)
 {
-    gen_arith(ctx, OPC_RISC_SRL, a->rd, a->rs1, a->rs2);
-    return true;
+    return gen_shift(ctx, a, &tcg_gen_shr_tl);
 }
 
 static bool trans_sra(DisasContext *ctx, arg_sra *a)
 {
-    gen_arith(ctx, OPC_RISC_SRA, a->rd, a->rs1, a->rs2);
-    return true;
+    return gen_shift(ctx, a, &tcg_gen_sar_tl);
 }
 
 static bool trans_or(DisasContext *ctx, arg_or *a)
@@ -410,19 +427,58 @@  static bool trans_subw(DisasContext *ctx, arg_subw *a)
 
 static bool trans_sllw(DisasContext *ctx, arg_sllw *a)
 {
-    gen_arith(ctx, OPC_RISC_SLLW, a->rd, a->rs1, a->rs2);
+    TCGv source1 = tcg_temp_new();
+    TCGv source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    gen_get_gpr(source2, a->rs2);
+
+    tcg_gen_andi_tl(source2, source2, 0x1F);
+    tcg_gen_shl_tl(source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
     return true;
 }
 
 static bool trans_srlw(DisasContext *ctx, arg_srlw *a)
 {
-    gen_arith(ctx, OPC_RISC_SRLW, a->rd, a->rs1, a->rs2);
+    TCGv source1 = tcg_temp_new();
+    TCGv source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    gen_get_gpr(source2, a->rs2);
+
+    /* clear upper 32 */
+    tcg_gen_ext32u_tl(source1, source1);
+    tcg_gen_andi_tl(source2, source2, 0x1F);
+    tcg_gen_shr_tl(source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
     return true;
 }
 
 static bool trans_sraw(DisasContext *ctx, arg_sraw *a)
 {
-    gen_arith(ctx, OPC_RISC_SRAW, a->rd, a->rs1, a->rs2);
+    TCGv source1 = tcg_temp_new();
+    TCGv source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    gen_get_gpr(source2, a->rs2);
+
+    /* first, trick to get it to act like working on 32 bits (get rid of
+       upper 32, sign extend to fill space) */
+    tcg_gen_ext32s_tl(source1, source1);
+    tcg_gen_andi_tl(source2, source2, 0x1F);
+    tcg_gen_sar_tl(source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
+
     return true;
 }
 #endif
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index a781aba08c..5b4d091561 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -192,47 +192,6 @@  static void gen_arith(DisasContext *ctx, uint32_t opc, int rd, int rs1,
     gen_get_gpr(source2, rs2);
 
     switch (opc) {
-#if defined(TARGET_RISCV64)
-    case OPC_RISC_SLLW:
-        tcg_gen_andi_tl(source2, source2, 0x1F);
-        tcg_gen_shl_tl(source1, source1, source2);
-        break;
-#endif
-    case OPC_RISC_SLL:
-        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
-        tcg_gen_shl_tl(source1, source1, source2);
-        break;
-    case OPC_RISC_SLT:
-        tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);
-        break;
-    case OPC_RISC_SLTU:
-        tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);
-        break;
-#if defined(TARGET_RISCV64)
-    case OPC_RISC_SRLW:
-        /* clear upper 32 */
-        tcg_gen_ext32u_tl(source1, source1);
-        tcg_gen_andi_tl(source2, source2, 0x1F);
-        tcg_gen_shr_tl(source1, source1, source2);
-        break;
-#endif
-    case OPC_RISC_SRL:
-        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
-        tcg_gen_shr_tl(source1, source1, source2);
-        break;
-#if defined(TARGET_RISCV64)
-    case OPC_RISC_SRAW:
-        /* first, trick to get it to act like working on 32 bits (get rid of
-        upper 32, sign extend to fill space) */
-        tcg_gen_ext32s_tl(source1, source1);
-        tcg_gen_andi_tl(source2, source2, 0x1F);
-        tcg_gen_sar_tl(source1, source1, source2);
-        break;
-#endif
-    case OPC_RISC_SRA:
-        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
-        tcg_gen_sar_tl(source1, source1, source2);
-        break;
     CASE_OP_32_64(OPC_RISC_MUL):
         tcg_gen_mul_tl(source1, source1, source2);
         break;
@@ -647,6 +606,24 @@  static bool trans_arith(DisasContext *ctx, arg_r *a,
     return true;
 }
 
+static bool gen_shift(DisasContext *ctx, arg_r *a,
+                        void(*func)(TCGv, TCGv, TCGv))
+{
+    TCGv source1 = tcg_temp_new();
+    TCGv source2 = tcg_temp_new();
+
+    gen_get_gpr(source1, a->rs1);
+    gen_get_gpr(source2, a->rs2);
+
+    tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
+    (*func)(source1, source1, source2);
+
+    gen_set_gpr(a->rd, source1);
+    tcg_temp_free(source1);
+    tcg_temp_free(source2);
+    return true;
+}
+
 /* Include insn module translation function */
 #include "insn_trans/trans_rvi.inc.c"
 #include "insn_trans/trans_rvm.inc.c"