Re: [Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding

2018-11-06 Thread Palmer Dabbelt 

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.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.


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

Re: [Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding

2018-11-05 Thread Bastian Koppelmann 



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

Re: [Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding

2018-11-01 Thread Palmer Dabbelt 

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.


Sorry for adding a bunch of work for you, Bastian :)

Re: [Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding

2018-10-31 Thread Richard Henderson 
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~