On Fri, Nov 20, 2020 at 5:04 AM Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 11/19/20 12:35 PM, Richard Henderson wrote:
> > On 11/18/20 12:29 AM, frank.ch...@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
