Re: [Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding
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
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
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
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~
[Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding
Reviewed-by: Richard Henderson
Signed-off-by: Bastian Koppelmann
Signed-off-by: Peer Adelt
---
target/riscv/insn_trans/trans_rvi.inc.c | 78 +
target/riscv/translate.c| 59 ++-
2 files changed, 85 insertions(+), 52 deletions(-)
diff --git a/target/riscv/insn_trans/trans_rvi.inc.c
b/target/riscv/insn_trans/trans_rvi.inc.c
index 5b554a..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, _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, _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, _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,
