Re: [RFC v3 08/16] target/riscv: add gen_shifti() and gen_shiftiw() helper functions

[Frank Chang]

On Tue, Jan 12, 2021 at 12:54 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 1/11/21 4:19 PM, frank.ch...@sifive.com wrote:
> >  static bool trans_slli(DisasContext *ctx, arg_slli *a)
> >  {
> > -if (a->shamt >= TARGET_LONG_BITS) {
> > -return false;
> > -}
> > -
> >  if (a->rd != 0) {
> > -TCGv t = tcg_temp_new();
> > -gen_get_gpr(t, a->rs1);
> > -
> > -tcg_gen_shli_tl(t, t, a->shamt);
> > -
> > -gen_set_gpr(a->rd, t);
> > -tcg_temp_free(t);
> > +return gen_shifti(ctx, a, tcg_gen_shl_tl);
> >  } /* NOP otherwise */
> >  return true;
> >  }
> >
> >  static bool trans_srli(DisasContext *ctx, arg_srli *a)
> >  {
> > -if (a->shamt >= TARGET_LONG_BITS) {
> > -return false;
> > -}
> > -
> >  if (a->rd != 0) {
> > -TCGv t = tcg_temp_new();
> > -gen_get_gpr(t, a->rs1);
> > -
> > -tcg_gen_shri_tl(t, t, a->shamt);
> > -gen_set_gpr(a->rd, t);
> > -tcg_temp_free(t);
> > +return gen_shifti(ctx, a, tcg_gen_shr_tl);
> >  } /* NOP otherwise */
> >  return true;
> >  }
> >
> >  static bool trans_srai(DisasContext *ctx, arg_srai *a)
> >  {
> > -if (a->shamt >= TARGET_LONG_BITS) {
> > -return false;
> > -}
> > -
> >  if (a->rd != 0) {
> > -TCGv t = tcg_temp_new();
> > -gen_get_gpr(t, a->rs1);
> > -
> > -tcg_gen_sari_tl(t, t, a->shamt);
> > -gen_set_gpr(a->rd, t);
> > -tcg_temp_free(t);
> > +return gen_shifti(ctx, a, tcg_gen_sar_tl);
> >  } /* NOP otherwise */
> >  return true;
> >  }
>
> This removes the illegal instruction check for rd == 0.
>
> In general you don't need the rd != 0 check, because gen_set_gpr will
> handle it
> (and it'll be exceedingly rare, and therefore not worth checking by hand).
>

> r~
>

Sure, I'll remove a->rd != 0 check in the next patchset.
Thanks,

Frank Chang

Re: [RFC v3 08/16] target/riscv: add gen_shifti() and gen_shiftiw() helper functions

[Richard Henderson]

On 1/11/21 4:19 PM, frank.ch...@sifive.com wrote:
>  static bool trans_slli(DisasContext *ctx, arg_slli *a)
>  {
> -if (a->shamt >= TARGET_LONG_BITS) {
> -return false;
> -}
> -
>  if (a->rd != 0) {
> -TCGv t = tcg_temp_new();
> -gen_get_gpr(t, a->rs1);
> -
> -tcg_gen_shli_tl(t, t, a->shamt);
> -
> -gen_set_gpr(a->rd, t);
> -tcg_temp_free(t);
> +return gen_shifti(ctx, a, tcg_gen_shl_tl);
>  } /* NOP otherwise */
>  return true;
>  }
>  
>  static bool trans_srli(DisasContext *ctx, arg_srli *a)
>  {
> -if (a->shamt >= TARGET_LONG_BITS) {
> -return false;
> -}
> -
>  if (a->rd != 0) {
> -TCGv t = tcg_temp_new();
> -gen_get_gpr(t, a->rs1);
> -
> -tcg_gen_shri_tl(t, t, a->shamt);
> -gen_set_gpr(a->rd, t);
> -tcg_temp_free(t);
> +return gen_shifti(ctx, a, tcg_gen_shr_tl);
>  } /* NOP otherwise */
>  return true;
>  }
>  
>  static bool trans_srai(DisasContext *ctx, arg_srai *a)
>  {
> -if (a->shamt >= TARGET_LONG_BITS) {
> -return false;
> -}
> -
>  if (a->rd != 0) {
> -TCGv t = tcg_temp_new();
> -gen_get_gpr(t, a->rs1);
> -
> -tcg_gen_sari_tl(t, t, a->shamt);
> -gen_set_gpr(a->rd, t);
> -tcg_temp_free(t);
> +return gen_shifti(ctx, a, tcg_gen_sar_tl);
>  } /* NOP otherwise */
>  return true;
>  }

This removes the illegal instruction check for rd == 0.

In general you don't need the rd != 0 check, because gen_set_gpr will handle it
(and it'll be exceedingly rare, and therefore not worth checking by hand).


r~

[RFC v3 08/16] target/riscv: add gen_shifti() and gen_shiftiw() helper functions

[frank . chang]

From: Frank Chang 

Add gen_shifti() and gen_shiftiw() helper functions to reuse the same
interfaces for immediate shift instructions.

Signed-off-by: Frank Chang 
---
 target/riscv/insn_trans/trans_rvi.c.inc | 45 +++--
 target/riscv/translate.c| 43 +++
 2 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvi.c.inc 
b/target/riscv/insn_trans/trans_rvi.c.inc
index d04ca0394cf..678c8f07238 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -261,52 +261,24 @@ static bool trans_andi(DisasContext *ctx, arg_andi *a)
 }
 static bool trans_slli(DisasContext *ctx, arg_slli *a)
 {
-if (a->shamt >= TARGET_LONG_BITS) {
-return false;
-}
-
 if (a->rd != 0) {
-TCGv t = tcg_temp_new();
-gen_get_gpr(t, a->rs1);
-
-tcg_gen_shli_tl(t, t, a->shamt);
-
-gen_set_gpr(a->rd, t);
-tcg_temp_free(t);
+return gen_shifti(ctx, a, tcg_gen_shl_tl);
 } /* NOP otherwise */
 return true;
 }
 
 static bool trans_srli(DisasContext *ctx, arg_srli *a)
 {
-if (a->shamt >= TARGET_LONG_BITS) {
-return false;
-}
-
 if (a->rd != 0) {
-TCGv t = tcg_temp_new();
-gen_get_gpr(t, a->rs1);
-
-tcg_gen_shri_tl(t, t, a->shamt);
-gen_set_gpr(a->rd, t);
-tcg_temp_free(t);
+return gen_shifti(ctx, a, tcg_gen_shr_tl);
 } /* NOP otherwise */
 return true;
 }
 
 static bool trans_srai(DisasContext *ctx, arg_srai *a)
 {
-if (a->shamt >= TARGET_LONG_BITS) {
-return false;
-}
-
 if (a->rd != 0) {
-TCGv t = tcg_temp_new();
-gen_get_gpr(t, a->rs1);
-
-tcg_gen_sari_tl(t, t, a->shamt);
-gen_set_gpr(a->rd, t);
-tcg_temp_free(t);
+return gen_shifti(ctx, a, tcg_gen_sar_tl);
 } /* NOP otherwise */
 return true;
 }
@@ -369,16 +341,7 @@ static bool trans_addiw(DisasContext *ctx, arg_addiw *a)
 
 static bool trans_slliw(DisasContext *ctx, arg_slliw *a)
 {
-TCGv source1;
-source1 = tcg_temp_new();
-gen_get_gpr(source1, a->rs1);
-
-tcg_gen_shli_tl(source1, source1, a->shamt);
-tcg_gen_ext32s_tl(source1, source1);
-gen_set_gpr(a->rd, source1);
-
-tcg_temp_free(source1);
-return true;
+return gen_shiftiw(ctx, a, tcg_gen_shl_tl);
 }
 
 static bool trans_srliw(DisasContext *ctx, arg_srliw *a)
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 53c0c34ce16..8459b6bcf54 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -810,6 +810,49 @@ static bool gen_shift(DisasContext *ctx, arg_r *a,
 return true;
 }
 
+static bool gen_shifti(DisasContext *ctx, arg_shift *a,
+   void(*func)(TCGv, TCGv, TCGv))
+{
+if (a->shamt >= TARGET_LONG_BITS) {
+return false;
+}
+
+TCGv source1 = tcg_temp_new();
+TCGv source2 = tcg_temp_new();
+
+gen_get_gpr(source1, a->rs1);
+
+tcg_gen_movi_tl(source2, a->shamt);
+(*func)(source1, source1, source2);
+
+gen_set_gpr(a->rd, source1);
+tcg_temp_free(source1);
+tcg_temp_free(source2);
+return true;
+}
+
+#ifdef TARGET_RISCV64
+
+static bool gen_shiftiw(DisasContext *ctx, arg_shift *a,
+void(*func)(TCGv, TCGv, TCGv))
+{
+TCGv source1 = tcg_temp_new();
+TCGv source2 = tcg_temp_new();
+
+gen_get_gpr(source1, a->rs1);
+tcg_gen_movi_tl(source2, a->shamt);
+
+(*func)(source1, source1, source2);
+tcg_gen_ext32s_tl(source1, source1);
+
+gen_set_gpr(a->rd, source1);
+tcg_temp_free(source1);
+tcg_temp_free(source2);
+return true;
+}
+
+#endif
+
 static void gen_ctz(TCGv ret, TCGv arg1)
 {
 tcg_gen_ctzi_tl(ret, arg1, TARGET_LONG_BITS);
-- 
2.17.1