Re: [PATCH 5/5] target/ppc: Implement paddi and replace addi insns - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Luis Pires <luis.pires@eldorado.org.br>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org
Cc: lagarcia@br.ibm.com, bruno.larsen@eldorado.org.br,
	richard.henderson@linaro.org, matheus.ferst@eldorado.org.br,
	david@gibson.dropbear.id.au
Subject: Re: [PATCH 5/5] target/ppc: Implement paddi and replace addi insns
Date: Wed, 14 Apr 2021 00:41:21 +0200	[thread overview]
Message-ID: <866f5277-60e9-3934-2465-e93c6d5f74b0@amsat.org> (raw)
In-Reply-To: <20210413211129.457272-6-luis.pires@eldorado.org.br>

Hi Luis,

On 4/13/21 11:11 PM, Luis Pires wrote:
> This implements the Power ISA 3.1 prefixed (64-bit) paddi
> instruction, while also replacing the legacy addi implementation.
> Both using the decode tree.
> 
> Signed-off-by: Luis Pires <luis.pires@eldorado.org.br>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>  target/ppc/ppc.decode                      |  8 +++++++
>  target/ppc/translate.c                     | 15 +------------
>  target/ppc/translate/fixedpoint-impl.c.inc | 26 ++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 14 deletions(-)
>  create mode 100644 target/ppc/translate/fixedpoint-impl.c.inc
> 
> diff --git a/target/ppc/ppc.decode b/target/ppc/ppc.decode
> index 84bb73ac19..c87174dc20 100644
> --- a/target/ppc/ppc.decode
> +++ b/target/ppc/ppc.decode
> @@ -16,3 +16,11 @@
>  # You should have received a copy of the GNU Lesser General Public
>  # License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  #
> +
> +%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
}

> +addi    001110 rt:5 ra:5 si:s16 &addi r=0
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 0f123f7b80..2ff192c9e5 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -945,19 +945,6 @@ GEN_INT_ARITH_ADD(addex, 0x05, cpu_ov, 1, 1, 0);
>  /* addze  addze.  addzeo  addzeo.*/
>  GEN_INT_ARITH_ADD_CONST(addze, 0x06, 0, cpu_ca, 1, 1, 0)
>  GEN_INT_ARITH_ADD_CONST(addzeo, 0x16, 0, cpu_ca, 1, 1, 1)
> -/* addi */
> -static void gen_addi(DisasContext *ctx)
> -{
> -    target_long simm = SIMM(ctx->opcode);
> -
> -    if (rA(ctx->opcode) == 0) {
> -        /* li case */
> -        tcg_gen_movi_tl(cpu_gpr[rD(ctx->opcode)], simm);
> -    } else {
> -        tcg_gen_addi_tl(cpu_gpr[rD(ctx->opcode)],
> -                        cpu_gpr[rA(ctx->opcode)], simm);
> -    }
> -}
>  /* addic  addic.*/
>  static inline void gen_op_addic(DisasContext *ctx, bool compute_rc0)
>  {
> @@ -6967,6 +6954,7 @@ static target_ulong ppc_peek_next_insn_size(DisasContext *ctx)
>  }
>  
>  #include "decode-ppc.c.inc"
> +#include "translate/fixedpoint-impl.c.inc"
>  
>  #include "translate/fp-impl.c.inc"
>  
> @@ -7091,7 +7079,6 @@ GEN_HANDLER_E(cmpeqb, 0x1F, 0x00, 0x07, 0x00600000, PPC_NONE, PPC2_ISA300),
>  GEN_HANDLER_E(cmpb, 0x1F, 0x1C, 0x0F, 0x00000001, PPC_NONE, PPC2_ISA205),
>  GEN_HANDLER_E(cmprb, 0x1F, 0x00, 0x06, 0x00400001, PPC_NONE, PPC2_ISA300),
>  GEN_HANDLER(isel, 0x1F, 0x0F, 0xFF, 0x00000001, PPC_ISEL),
> -GEN_HANDLER(addi, 0x0E, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
>  GEN_HANDLER(addic, 0x0C, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
>  GEN_HANDLER2(addic_, "addic.", 0x0D, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
>  GEN_HANDLER(addis, 0x0F, 0xFF, 0xFF, 0x00000000, PPC_INTEGER),
> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> new file mode 100644
> index 0000000000..8620954b57
> --- /dev/null
> +++ 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).

> +{
> +    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.

Regards,

Phil.