On 11/1/18 4:59 PM, Palmer Dabbelt wrote:
On Wed, 31 Oct 2018 15:38:08 PDT (-0700), richard.hender...@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



Reply via email to