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

[Frank Chang <frank.chang@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 5214 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 6903 bytes --]