diff mbox series

[RFC,08/15] target/riscv: rvb: single-bit instructions

Message ID 20201118083044.13992-9-frank.chang@sifive.com
State New
Headers show
Series support subsets of bitmanip extension | expand

Commit Message

Frank Chang Nov. 18, 2020, 8:29 a.m. UTC 
From: Kito Cheng <kito.cheng@sifive.com>

Signed-off-by: Kito Cheng <kito.cheng@sifive.com>
---
 target/riscv/insn32-64.decode           |   8 ++
 target/riscv/insn32.decode              |   9 ++
 target/riscv/insn_trans/trans_rvb.c.inc |  90 ++++++++++++++
 target/riscv/translate.c                | 155 ++++++++++++++++++++++++
 4 files changed, 262 insertions(+)

Comments

Richard Henderson Nov. 19, 2020, 8:05 p.m. UTC | #1 
On 11/18/20 12:29 AM, frank.chang@sifive.com wrote:
> +static void gen_sbop_shamt(TCGv ret, TCGv shamt)
> +{
> +    tcg_gen_andi_tl(ret, shamt, TARGET_LONG_BITS - 1);
> +}
> +
> +static void gen_sbop_common(TCGv ret, TCGv shamt)
> +{
> +    TCGv t;
> +    t = tcg_temp_new();

All of the places where you declare then initialize on the next line, please
merge them:

   TCGv t = tcg_temp_new();

It would be nice to share more code between the normal and *w versions.  As it
is, there's a *lot* of repetition with only TARGET_LONG_BITS vs 32 separating them.

> +    tcg_gen_not_tl(mask, mask);
> +    tcg_gen_and_tl(ret, arg1, mask);

andc.


r~
Richard Henderson Nov. 19, 2020, 8:35 p.m. UTC | #2 
On 11/18/20 12:29 AM, frank.chang@sifive.com wrote:
> +static bool trans_sbset(DisasContext *ctx, arg_sbset *a)
> +{
> +    REQUIRE_EXT(ctx, RVB);
> +    return gen_arith(ctx, a, &gen_sbset);
> +}
> +
> +static bool trans_sbseti(DisasContext *ctx, arg_sbseti *a)
> +{
> +    REQUIRE_EXT(ctx, RVB);
> +    return gen_arith_shamt_tl(ctx, a, &gen_sbset);
> +}
> +
> +static bool trans_sbclr(DisasContext *ctx, arg_sbclr *a)
> +{
> +    REQUIRE_EXT(ctx, RVB);
> +    return gen_arith(ctx, a, &gen_sbclr);
> +}

Coming back to my re-use of code thing, these should use gen_shift.  That
handles the truncate of source2 to the shift amount.

> +static bool trans_sbclri(DisasContext *ctx, arg_sbclri *a)
> +{
> +    REQUIRE_EXT(ctx, RVB);
> +    return gen_arith_shamt_tl(ctx, a, &gen_sbclr);
> +}
> +
> +static bool trans_sbinv(DisasContext *ctx, arg_sbinv *a)
> +{
> +    REQUIRE_EXT(ctx, RVB);
> +    return gen_arith(ctx, a, &gen_sbinv);
> +}
> +
> +static bool trans_sbinvi(DisasContext *ctx, arg_sbinvi *a)
> +{
> +    REQUIRE_EXT(ctx, RVB);
> +    return gen_arith_shamt_tl(ctx, a, &gen_sbinv);
> +}

I think there ought to be a gen_shifti for these.

Similarly gen_shiftiw and gen_shiftw, which would truncate and sign-extend the
result.  Existing code in trans_rvi.c.inc should be converted to use the new
functions.


r~
Richard Henderson Nov. 19, 2020, 9:04 p.m. UTC | #3 
On 11/19/20 12:35 PM, Richard Henderson wrote:
> On 11/18/20 12:29 AM, frank.chang@sifive.com wrote:
>> +static bool trans_sbset(DisasContext *ctx, arg_sbset *a)
>> +{
>> +    REQUIRE_EXT(ctx, RVB);
>> +    return gen_arith(ctx, a, &gen_sbset);
>> +}
>> +
>> +static bool trans_sbseti(DisasContext *ctx, arg_sbseti *a)
>> +{
>> +    REQUIRE_EXT(ctx, RVB);
>> +    return gen_arith_shamt_tl(ctx, a, &gen_sbset);
>> +}
>> +
>> +static bool trans_sbclr(DisasContext *ctx, arg_sbclr *a)
>> +{
>> +    REQUIRE_EXT(ctx, RVB);
>> +    return gen_arith(ctx, a, &gen_sbclr);
>> +}
> 
> Coming back to my re-use of code thing, these should use gen_shift.  That
> handles the truncate of source2 to the shift amount.
> 
>> +static bool trans_sbclri(DisasContext *ctx, arg_sbclri *a)
>> +{
>> +    REQUIRE_EXT(ctx, RVB);
>> +    return gen_arith_shamt_tl(ctx, a, &gen_sbclr);
>> +}
>> +
>> +static bool trans_sbinv(DisasContext *ctx, arg_sbinv *a)
>> +{
>> +    REQUIRE_EXT(ctx, RVB);
>> +    return gen_arith(ctx, a, &gen_sbinv);
>> +}
>> +
>> +static bool trans_sbinvi(DisasContext *ctx, arg_sbinvi *a)
>> +{
>> +    REQUIRE_EXT(ctx, RVB);
>> +    return gen_arith_shamt_tl(ctx, a, &gen_sbinv);
>> +}
> 
> I think there ought to be a gen_shifti for these.

Hmm.  I just realized that gen_shifti would have a generator callback with a
constant argument, a-la tcg_gen_shli_tl.

I don't know if it's worth duplicating gen_sbclr et al for a constant argument.
 And the sloi/sroi insns besides.  Perhaps a gen_shifti_var helper instead?

Let me know what you think, but at the moment we're left with an incoherent set
of helpers that seem split along lines that are less than ideal.


r~
Frank Chang Dec. 4, 2020, 5:10 p.m. UTC | #4 
On Fri, Nov 20, 2020 at 5:04 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/19/20 12:35 PM, Richard Henderson wrote:
> > On 11/18/20 12:29 AM, frank.chang@sifive.com wrote:
> >> +static bool trans_sbset(DisasContext *ctx, arg_sbset *a)
> >> +{
> >> +    REQUIRE_EXT(ctx, RVB);
> >> +    return gen_arith(ctx, a, &gen_sbset);
> >> +}
> >> +
> >> +static bool trans_sbseti(DisasContext *ctx, arg_sbseti *a)
> >> +{
> >> +    REQUIRE_EXT(ctx, RVB);
> >> +    return gen_arith_shamt_tl(ctx, a, &gen_sbset);
> >> +}
> >> +
> >> +static bool trans_sbclr(DisasContext *ctx, arg_sbclr *a)
> >> +{
> >> +    REQUIRE_EXT(ctx, RVB);
> >> +    return gen_arith(ctx, a, &gen_sbclr);
> >> +}
> >
> > Coming back to my re-use of code thing, these should use gen_shift.  That
> > handles the truncate of source2 to the shift amount.
> >
> >> +static bool trans_sbclri(DisasContext *ctx, arg_sbclri *a)
> >> +{
> >> +    REQUIRE_EXT(ctx, RVB);
> >> +    return gen_arith_shamt_tl(ctx, a, &gen_sbclr);
> >> +}
> >> +
> >> +static bool trans_sbinv(DisasContext *ctx, arg_sbinv *a)
> >> +{
> >> +    REQUIRE_EXT(ctx, RVB);
> >> +    return gen_arith(ctx, a, &gen_sbinv);
> >> +}
> >> +
> >> +static bool trans_sbinvi(DisasContext *ctx, arg_sbinvi *a)
> >> +{
> >> +    REQUIRE_EXT(ctx, RVB);
> >> +    return gen_arith_shamt_tl(ctx, a, &gen_sbinv);
> >> +}
> >
> > I think there ought to be a gen_shifti for these.
>
> Hmm.  I just realized that gen_shifti would have a generator callback with
> a
> constant argument, a-la tcg_gen_shli_tl.
>
> I don't know if it's worth duplicating gen_sbclr et al for a constant
> argument.
>  And the sloi/sroi insns besides.  Perhaps a gen_shifti_var helper instead?
>
> Let me know what you think, but at the moment we're left with an
> incoherent set
> of helpers that seem split along lines that are less than ideal.
>
>
> r~
>

Thanks Richard and sorry for the late reply.....

If we can have gen_shift(), gen_shifti(), gen_shiftw() and gen_shiftiw(),
then we can eliminate the needs of:
gen_arith_shamt_tl(), gen_sbop_shamt(), gen_sbopw_shamt()
and gen_sbopw_common()
and most of the *w version generators can be removed, too.

For *w version, we just need to call gen_shiftw() or gen_shiftiw()
with the reused non-*w version generator.
For example:

  static bool trans_sbclrw(DisasContext *ctx, arg_sbclrw *a)
  {
      REQUIRE_EXT(ctx, RVB);
      return gen_shiftw(ctx, a, &gen_sbclr);
  }

  static bool trans_sbclriw(DisasContext *ctx, arg_sbclriw *a)
  {
      REQUIRE_EXT(ctx, RVB);
      return gen_shiftiw(ctx, a, &gen_sbclr);
  }

both of which can reuse gen_sbclr() generator:

  static void gen_sbclr(TCGv ret, TCGv arg1, TCGv shamt)
  {
      TCGv t = tcg_temp_new();
      tcg_gen_movi_tl(t, 1);
      tcg_gen_shl_tl(t, t, shamt);
      tcg_gen_andc_tl(ret, arg1, t);
      tcg_temp_free(t);
  }

The gen_shift*() I have now are as follow:

  static bool gen_shift(DisasContext *ctx, arg_r *a,
                          void(*func)(TCGv, TCGv, TCGv))
  {
      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, TARGET_LONG_BITS - 1);
      (*func)(source1, source1, source2);

      gen_set_gpr(a->rd, source1);
      tcg_temp_free(source1);
      tcg_temp_free(source2);
      return true;
  }

  static bool gen_shifti(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);

      tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
      (*func)(source1, source1, source2);

      gen_set_gpr(a->rd, source1);
      tcg_temp_free(source1);
      tcg_temp_free(source2);
      return true;
  }

  static bool gen_shiftw(DisasContext *ctx, arg_r *a,
                          void(*func)(TCGv, TCGv, TCGv))
  {
      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, 31);
      (*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;
  }

  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);

     tcg_gen_andi_tl(source2, source2, 31);
     (*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;
  }

They may be further merged as most of them are duplicate with only the
differences of:
gen_get_gpr(source2, a->rs2); vs. tcg_gen_movi_tl(source2, a->shamt);
TARGET_LONG_BITS - 1 vs. 31, and
tcg_gen_ext32s_tl(); to sign-extend the 32-bit return value for *w
instructions

Any thoughts?

Frank Chang
diff mbox series

Patch

diff --git a/target/riscv/insn32-64.decode b/target/riscv/insn32-64.decode
index 2f00f96e36b..92f3aaac3b6 100644
--- a/target/riscv/insn32-64.decode
+++ b/target/riscv/insn32-64.decode
@@ -94,3 +94,11 @@  pcntw      011000000010 ..... 001 ..... 0011011 @r2
 
 packw      0000100 .......... 100 ..... 0111011 @r
 packuw     0100100 .......... 100 ..... 0111011 @r
+sbsetw     0010100 .......... 001 ..... 0111011 @r
+sbclrw     0100100 .......... 001 ..... 0111011 @r
+sbinvw     0110100 .......... 001 ..... 0111011 @r
+sbextw     0100100 .......... 101 ..... 0111011 @r
+
+sbsetiw    0010100 .......... 001 ..... 0011011 @sh5
+sbclriw    0100100 .......... 001 ..... 0011011 @sh5
+sbinviw    0110100 .......... 001 ..... 0011011 @sh5
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
index 626641333c6..69e542da19c 100644
--- a/target/riscv/insn32.decode
+++ b/target/riscv/insn32.decode
@@ -611,3 +611,12 @@  min        0000101 .......... 100 ..... 0110011 @r
 minu       0000101 .......... 101 ..... 0110011 @r
 max        0000101 .......... 110 ..... 0110011 @r
 maxu       0000101 .......... 111 ..... 0110011 @r
+sbset      0010100 .......... 001 ..... 0110011 @r
+sbclr      0100100 .......... 001 ..... 0110011 @r
+sbinv      0110100 .......... 001 ..... 0110011 @r
+sbext      0100100 .......... 101 ..... 0110011 @r
+
+sbseti     001010 ........... 001 ..... 0010011 @sh
+sbclri     010010 ........... 001 ..... 0010011 @sh
+sbinvi     011010 ........... 001 ..... 0010011 @sh
+sbexti     010010 ........... 101 ..... 0010011 @sh
diff --git a/target/riscv/insn_trans/trans_rvb.c.inc b/target/riscv/insn_trans/trans_rvb.c.inc
index bf15611f85a..dabf8e09c3d 100644
--- a/target/riscv/insn_trans/trans_rvb.c.inc
+++ b/target/riscv/insn_trans/trans_rvb.c.inc
@@ -107,6 +107,54 @@  static bool trans_sext_h(DisasContext *ctx, arg_sext_h *a)
     return gen_unary(ctx, a, &tcg_gen_ext16s_tl);
 }
 
+static bool trans_sbset(DisasContext *ctx, arg_sbset *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbset);
+}
+
+static bool trans_sbseti(DisasContext *ctx, arg_sbseti *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbset);
+}
+
+static bool trans_sbclr(DisasContext *ctx, arg_sbclr *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbclr);
+}
+
+static bool trans_sbclri(DisasContext *ctx, arg_sbclri *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbclr);
+}
+
+static bool trans_sbinv(DisasContext *ctx, arg_sbinv *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbinv);
+}
+
+static bool trans_sbinvi(DisasContext *ctx, arg_sbinvi *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbinv);
+}
+
+static bool trans_sbext(DisasContext *ctx, arg_sbext *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbext);
+}
+
+static bool trans_sbexti(DisasContext *ctx, arg_sbexti *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbext);
+}
+
 {
 /* RV64-only instructions */
 #ifdef TARGET_RISCV64
@@ -141,4 +189,46 @@  static bool trans_packuw(DisasContext *ctx, arg_packuw *a)
     return gen_arith(ctx, a, &gen_packuw);
 }
 
+static bool trans_sbsetw(DisasContext *ctx, arg_sbsetw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbsetw);
+}
+
+static bool trans_sbsetiw(DisasContext *ctx, arg_sbsetiw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbsetw);
+}
+
+static bool trans_sbclrw(DisasContext *ctx, arg_sbclrw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbclrw);
+}
+
+static bool trans_sbclriw(DisasContext *ctx, arg_sbclriw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbclrw);
+}
+
+static bool trans_sbinvw(DisasContext *ctx, arg_sbinvw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbinvw);
+}
+
+static bool trans_sbinviw(DisasContext *ctx, arg_sbinviw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith_shamt_tl(ctx, a, &gen_sbinvw);
+}
+
+static bool trans_sbextw(DisasContext *ctx, arg_sbextw *a)
+{
+    REQUIRE_EXT(ctx, RVB);
+    return gen_arith(ctx, a, &gen_sbextw);
+}
+
 #endif
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index fb30ee83aa8..e7d9e4a1abf 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -647,6 +647,24 @@  static bool gen_arith_imm_tl(DisasContext *ctx, arg_i *a,
     return true;
 }
 
+static bool gen_arith_shamt_tl(DisasContext *ctx, arg_shift *a,
+                               void (*func)(TCGv, TCGv, TCGv))
+{
+    TCGv source1, source2;
+    source1 = tcg_temp_new();
+    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 void gen_addw(TCGv ret, TCGv arg1, TCGv arg2)
 {
@@ -801,6 +819,74 @@  static void gen_packh(TCGv ret, TCGv arg1, TCGv arg2)
     tcg_temp_free(higher);
 }
 
+static void gen_sbop_shamt(TCGv ret, TCGv shamt)
+{
+    tcg_gen_andi_tl(ret, shamt, TARGET_LONG_BITS - 1);
+}
+
+static void gen_sbop_common(TCGv ret, TCGv shamt)
+{
+    TCGv t;
+    t = tcg_temp_new();
+
+    gen_sbop_shamt(ret, shamt);
+
+    tcg_gen_movi_tl(t, 1);
+    tcg_gen_shl_tl(ret, t, ret);
+
+    tcg_temp_free(t);
+}
+
+static void gen_sbset(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv mask;
+    mask = tcg_temp_new();
+
+    gen_sbop_common(mask, arg2);
+
+    tcg_gen_or_tl(ret, arg1, mask);
+
+    tcg_temp_free(mask);
+}
+
+static void gen_sbclr(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv mask;
+    mask = tcg_temp_new();
+
+    gen_sbop_common(mask, arg2);
+
+    tcg_gen_not_tl(mask, mask);
+    tcg_gen_and_tl(ret, arg1, mask);
+
+    tcg_temp_free(mask);
+}
+
+static void gen_sbinv(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv mask;
+    mask = tcg_temp_new();
+
+    gen_sbop_common(mask, arg2);
+
+    tcg_gen_xor_tl(ret, arg1, mask);
+
+    tcg_temp_free(mask);
+}
+
+static void gen_sbext(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv shamt;
+    shamt = tcg_temp_new();
+
+    gen_sbop_shamt(shamt, arg2);
+    tcg_gen_shr_tl(ret, arg1, shamt);
+
+    tcg_gen_andi_tl(ret, ret, 1);
+
+    tcg_temp_free(shamt);
+}
+
 
 #ifdef TARGET_RISCV64
 
@@ -867,6 +953,75 @@  static void gen_packuw(TCGv ret, TCGv arg1, TCGv arg2)
     tcg_temp_free(higher);
 }
 
+static void gen_sbopw_shamt(TCGv ret, TCGv shamt)
+{
+    tcg_gen_andi_tl(ret, shamt, 31);
+}
+
+static void gen_sbopw_common(TCGv ret, TCGv shamt)
+{
+    TCGv t;
+    t = tcg_temp_new();
+
+    gen_sbopw_shamt(ret, shamt);
+    tcg_gen_movi_tl(t, 1);
+    tcg_gen_shl_tl(ret, t, ret);
+
+    tcg_temp_free(t);
+}
+
+static void gen_sbsetw(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv mask;
+    mask = tcg_temp_new();
+
+    gen_sbopw_common(mask, arg2);
+    tcg_gen_or_tl(ret, arg1, mask);
+
+    tcg_gen_ext32s_tl(ret, ret);
+
+    tcg_temp_free(mask);
+}
+
+static void gen_sbclrw(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv mask;
+    mask = tcg_temp_new();
+
+    gen_sbopw_common(mask, arg2);
+    tcg_gen_not_tl(mask, mask);
+    tcg_gen_and_tl(ret, arg1, mask);
+
+    tcg_gen_ext32s_tl(ret, ret);
+
+    tcg_temp_free(mask);
+}
+
+static void gen_sbinvw(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv mask;
+    mask = tcg_temp_new();
+
+    gen_sbopw_common(mask, arg2);
+    tcg_gen_xor_tl(ret, arg1, mask);
+
+    tcg_gen_ext32s_tl(ret, ret);
+
+    tcg_temp_free(mask);
+}
+
+static void gen_sbextw(TCGv ret, TCGv arg1, TCGv arg2)
+{
+    TCGv shamt;
+    shamt = tcg_temp_new();
+
+    gen_sbopw_shamt(shamt, arg2);
+    tcg_gen_shr_tl(ret, arg1, shamt);
+    tcg_gen_andi_tl(ret, ret, 1);
+
+    tcg_temp_free(shamt);
+}
+
 #endif
 
 static bool gen_arith(DisasContext *ctx, arg_r *a,