RE: [PATCH 5/5] target/ppc: Implement paddi and replace addi insns

From: Luis Fernando Fujita Pires <luis.pires@eldorado.org.br>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>
Cc: "lagarcia@br.ibm.com" <lagarcia@br.ibm.com>,
	Bruno Piazera Larsen <bruno.larsen@eldorado.org.br>,
	"richard.henderson@linaro.org" <richard.henderson@linaro.org>,
	Matheus Kowalczuk Ferst <matheus.ferst@eldorado.org.br>,
	"david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>
Subject: RE: [PATCH 5/5] target/ppc: Implement paddi and replace addi insns
Date: Wed, 14 Apr 2021 13:00:02 +0000	[thread overview]
Message-ID: <CP2PR80MB3668EB73292A9D659AA4117DDA4E9@CP2PR80MB3668.lamprd80.prod.outlook.com> (raw)
In-Reply-To: <866f5277-60e9-3934-2465-e93c6d5f74b0@amsat.org>

Hi Phil,

> > +
> > +%p_D8_SI        32:s18 0:16
> > +
> > +# Fixed-Point Facility Instructions
> > +&addi   r rt ra si
> > +
> > +paddi   000001 10 0 -- r:1 -- .................. 001110 rt:5 ra:5 ................
> si=%p_D8_SI &addi
> 
> IIUC you should be able to do something like catch ra=0 first ...:
> 
> {
>   addi_movi   000001 10 0 -- r:1 -- .................. 001110 rt:5 .....
> ................ si=%p_D8_SI &addi ra=0
>   addi   000001 10 0 -- r:1 -- .................. 001110 rt:5 ra:5
> ................ si=%p_D8_SI &addi
> }

> > +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> > @@ -0,0 +1,26 @@
> > +static bool trans_paddi(DisasContext *ctx, arg_paddi *a)
> 
> (Nitpick, use the format: arg_addi, not arg_paddi).

Sure, will do! I was using the exact function signature generated by
decodetree, but using 'arg_addi' makes more sense, as it will make
it clearer that it's the same struct.

> 
> > +{
> > +    if (a->r == 0) {
> > +        if (a->ra == 0) {
> > +            /* li case */
> > +            tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);
> > +        } else {
> > +            tcg_gen_addi_tl(cpu_gpr[a->rt],
> > +                            cpu_gpr[a->ra], a->si);
> > +        }
> > +    } else {
> > +        if (a->ra == 0) {
> > +            tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_nip, a->si);
> > +        } else {
> > +            /* invalid form */
> > +            gen_invalid(ctx);
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static bool trans_addi(DisasContext *ctx, arg_addi *a) {
> > +    return trans_paddi(ctx, a);
> > +}
> 
> ... which then simplifies the trans_OPCODE() logic:
> 
> static bool trans_addi_movi(DisasContext *ctx, arg_addi *a) {
>     if (a->r == 0) {
>         /* li case */
>         tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);
>     } else {
>         tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_nip, a->si);
>     }
> 
>     return true;
> }
> 
> static bool trans_addi(DisasContext *ctx, arg_addi *a) {
>     if (a->r == 0) {
>         tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], a->si);
>     } else {
>         /* invalid form */
>         gen_invalid(ctx);
>     }
> 
>     return true;
> }
> 
> Maybe you can do the same with the r bit to remove the dual addi_movi.

Right, that can be done. On the other hand, keeping this logic inside trans_paddi
and not in the .decode file makes it simpler (and less error-prone) to check that
the implementation matches the official ISA documentation, when opening
both side by side.

This is because the ISA specifies the instruction format for paddi as a whole
(which can be easily matched to what would be in the .decode file) and, after
that, the ISA specifies how each case should be handled (which could then
be checked by just looking at the trans_paddi implementation, which would
be in a single function).

Since the trans_paddi implementation with the ra/r handling is not that
complex either, I would recommend keeping the clearer separation
between the instruction formats (in the .decode file) and the
handling of each case in the trans_OPCODE() logic. What do you think?

Thanks,
Luis