Re: [PATCH v2 6/8] target/ppc: Move div/mod fixed-point insns (64 bits operands) to decodetree.

["Nicholas Piggin" <npiggin@gmail.com>]

On Sat May 18, 2024 at 8:48 PM AEST, Richard Henderson wrote:
> On 5/17/24 14:48, Nicholas Piggin wrote:
> > On Tue Apr 23, 2024 at 4:32 PM AEST, Chinmay Rath wrote:
> >> Moving the below instructions to decodetree specification :
> >>
> >> 	divd[u, e, eu][o][.]	: XO-form
> >> 	mod{sd, ud}		: X-form
> >>
> >> With this patch, all the fixed-point arithmetic instructions have been
> >> moved to decodetree.
> >> The changes were verified by validating that the tcg ops generated by those
> >> instructions remain the same, which were captured using the '-d in_asm,op' flag.
> >> Also, remaned do_divwe method in fixedpoint-impl.c.inc to do_dive because it is
> >> now used to divide doubleword operands as well, and not just words.
> >>
> >> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > 
> > [...]
> > 
> >> +static bool do_divd(DisasContext *ctx, arg_XO *a, bool sign)
> >> +{
> >> +    gen_op_arith_divd(ctx, cpu_gpr[a->rt], cpu_gpr[a->ra], cpu_gpr[a->rb],
> >> +                      sign, a->oe, a->rc);
> >> +    return true;
> >> +}
> >> +
> >> +static bool do_modd(DisasContext *ctx, arg_X *a, bool sign)
> >> +{
> >> +    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
> >> +    gen_op_arith_modd(ctx, cpu_gpr[a->rt], cpu_gpr[a->ra], cpu_gpr[a->rb],
> >> +                      sign);
> >> +    return true;
> >> +}
> >> +
> >> +TRANS64(DIVD, do_divd, true);
> >> +TRANS64(DIVDU, do_divd, false);
> >> +TRANS64(DIVDE, do_dive, gen_helper_DIVDE);
> >> +TRANS64(DIVDEU, do_dive, gen_helper_DIVDEU);
> >> +
> >> +TRANS64(MODSD, do_modd, true);
> >> +TRANS64(MODUD, do_modd, false);
> > 
> > Sigh. I'm having to fix a bunch of these for 32-bit builds. Just
> > doing the #ifdef TARGET_PPC64 ... #else qemu_build_not_reached();
> > thing.
> > 
> > Which is quite ugly and actually prevents using some of these
> > macros and requires open coding (e.g., because DIVDE helper is
> > not declared for 32-bit in this case).
>
> Compare sparc:
>
> # define gen_helper_pdist     ({ qemu_build_not_reached(); NULL; })
>
> etc.

That would help indeed.

>
> > Maybe we should move 64-bit only instructions into their own
> > .decode file and not build them for 32-bit, so we don't have
> > to add all these dummy translate functions for them.
>
> That's another option, yes.  The decodetree script will take multiple input files to 
> produce one output, so you could separate the insns by base vs 64-bit.

Thinking about it a bit more, I guess the downside is that you
would usually like to group instruction variants that operate on
64-bit data together with the others in the .decode file.

Thanks,
Nick