[2/2] target/riscv: Do amo*.w insns operate with 32 bits

Commit Message

LIU Zhiwei June 29, 2020, 1:07 p.m. UTC

For amo*.w insns, we should only calculate on the 32 bits data either from the
register or the memory.

Signed-off-by: LIU Zhiwei <[e-mail]>
---
 target/riscv/insn_trans/trans_rva.inc.c | 60 +++++++++++++++++++------
 1 file changed, 47 insertions(+), 13 deletions(-)

Comments

Richard Henderson June 30, 2020, 3 p.m. UTC | #1

On 6/29/20 6:07 AM, LIU Zhiwei wrote:
> +static bool
> +gen_amo_w(DisasContext *ctx, arg_atomic *a,
> +          void(*func)(TCGv, TCGv, TCGv, TCGArg, MemOp),
> +          MemOp mop, bool sign)
>  {
>      TCGv src1 = tcg_temp_new();
>      TCGv src2 = tcg_temp_new();
>  
>      gen_get_gpr(src1, a->rs1);
>      gen_get_gpr(src2, a->rs2);
> +    if (sign) {
> +        tcg_gen_ext32s_tl(src2, src2);
> +    } else {
> +        tcg_gen_ext32u_tl(src2, src2);
> +    }
>  
>      (*func)(src2, src1, src2, ctx->mem_idx, mop);
> -
> +    tcg_gen_ext32s_tl(src2, src2);
>      gen_set_gpr(a->rd, src2);
> +
>      tcg_temp_free(src1);
>      tcg_temp_free(src2);
>      return true;

With the fix to tcg, there should be no change required here, since you're
already passing MO_TESL for signed input.

Note that unsigned comparisions work as expected with sign-extended inputs.
That's what the risc-v isa does, after all.


r~

LIU Zhiwei June 30, 2020, 3:38 p.m. UTC | #2

On 2020/6/30 23:00, Richard Henderson wrote:
> On 6/29/20 6:07 AM, LIU Zhiwei wrote:
>> +static bool
>> +gen_amo_w(DisasContext *ctx, arg_atomic *a,
>> +          void(*func)(TCGv, TCGv, TCGv, TCGArg, MemOp),
>> +          MemOp mop, bool sign)
>>   {
>>       TCGv src1 = tcg_temp_new();
>>       TCGv src2 = tcg_temp_new();
>>   
>>       gen_get_gpr(src1, a->rs1);
>>       gen_get_gpr(src2, a->rs2);
>> +    if (sign) {
>> +        tcg_gen_ext32s_tl(src2, src2);
>> +    } else {
>> +        tcg_gen_ext32u_tl(src2, src2);
>> +    }
>>   
>>       (*func)(src2, src1, src2, ctx->mem_idx, mop);
>> -
>> +    tcg_gen_ext32s_tl(src2, src2);
>>       gen_set_gpr(a->rd, src2);
>> +
>>       tcg_temp_free(src1);
>>       tcg_temp_free(src2);
>>       return true;
> With the fix to tcg, there should be no change required here, since you're
> already passing MO_TESL for signed input.
>
> Note that unsigned comparisions work as expected with sign-extended inputs.
> That's what the risc-v isa does, after all.
>
Although some confusing,  it is right for unsigned comparisons. Thus the 
amominu.w will still
be calculated by

gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl,/(MO_ALIGN |//MO_TESL//)/);

In this way, the only fix is in tcg and this patch will be dropped.

Zhiwei

> r~

Patch

diff --git a/target/riscv/insn_trans/trans_rva.inc.c b/target/riscv/insn_trans/trans_rva.inc.c
index be8a9f06dd..6b3fc14436 100644
--- a/target/riscv/insn_trans/trans_rva.inc.c
+++ b/target/riscv/insn_trans/trans_rva.inc.c
@@ -81,19 +81,26 @@  static inline bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
     return true;
 }
 
-static bool gen_amo(DisasContext *ctx, arg_atomic *a,
-                    void(*func)(TCGv, TCGv, TCGv, TCGArg, MemOp),
-                    MemOp mop)
+static bool
+gen_amo_w(DisasContext *ctx, arg_atomic *a,
+          void(*func)(TCGv, TCGv, TCGv, TCGArg, MemOp),
+          MemOp mop, bool sign)
 {
     TCGv src1 = tcg_temp_new();
     TCGv src2 = tcg_temp_new();
 
     gen_get_gpr(src1, a->rs1);
     gen_get_gpr(src2, a->rs2);
+    if (sign) {
+        tcg_gen_ext32s_tl(src2, src2);
+    } else {
+        tcg_gen_ext32u_tl(src2, src2);
+    }
 
     (*func)(src2, src1, src2, ctx->mem_idx, mop);
-
+    tcg_gen_ext32s_tl(src2, src2);
     gen_set_gpr(a->rd, src2);
+
     tcg_temp_free(src1);
     tcg_temp_free(src2);
     return true;
@@ -114,59 +121,86 @@  static bool trans_sc_w(DisasContext *ctx, arg_sc_w *a)
 static bool trans_amoswap_w(DisasContext *ctx, arg_amoswap_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_xchg_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_xchg_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amoadd_w(DisasContext *ctx, arg_amoadd_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_add_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_add_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amoxor_w(DisasContext *ctx, arg_amoxor_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_xor_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_xor_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amoand_w(DisasContext *ctx, arg_amoand_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_and_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_and_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amoor_w(DisasContext *ctx, arg_amoor_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_or_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_or_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amomin_w(DisasContext *ctx, arg_amomin_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smin_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_smin_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amomax_w(DisasContext *ctx, arg_amomax_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_smax_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_smax_tl,
+                     (MO_ALIGN | MO_TESL), true);
 }
 
 static bool trans_amominu_w(DisasContext *ctx, arg_amominu_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umin_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_umin_tl,
+                     (MO_ALIGN | MO_TEUL), false);
 }
 
 static bool trans_amomaxu_w(DisasContext *ctx, arg_amomaxu_w *a)
 {
     REQUIRE_EXT(ctx, RVA);
-    return gen_amo(ctx, a, &tcg_gen_atomic_fetch_umax_tl, (MO_ALIGN | MO_TESL));
+    return gen_amo_w(ctx, a, &tcg_gen_atomic_fetch_umax_tl,
+                     (MO_ALIGN | MO_TEUL), false);
 }
 
 #ifdef TARGET_RISCV64
 
+static bool gen_amo(DisasContext *ctx, arg_atomic *a,
+                    void(*func)(TCGv_i64, TCGv_i64, TCGv_i64, TCGArg, MemOp),
+                    MemOp mop)
+{
+    TCGv src1 = tcg_temp_new();
+    TCGv src2 = tcg_temp_new();
+
+    gen_get_gpr(src1, a->rs1);
+    gen_get_gpr(src2, a->rs2);
+
+    (*func)(src2, src1, src2, ctx->mem_idx, mop);
+
+    gen_set_gpr(a->rd, src2);
+    tcg_temp_free(src1);
+    tcg_temp_free(src2);
+    return true;
+}
+
 static bool trans_lr_d(DisasContext *ctx, arg_lr_d *a)
 {
     return gen_lr(ctx, a, MO_ALIGN | MO_TEQ);