Re: [Qemu-devel] [PATCH v2 1/6] target/mips: Add MXU instructions S32I2M and S32M2I

[Aleksandar Markovic]

> From: Craig Janeczek <address@hidden>
> Sent: Monday, August 27, 2018 4:38 PM
> Subject: [PATCH v2 1/6] target/mips: Add MXU instructions S32I2M and S32M2I
> 
> This commit makes the MXU registers and the utility functions for
> reading/writing to them. This is required for full MXU instruction
> support.
> 
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek <address@hidden>
> ---
>  v1
>     - initial patch
>  v2
>     - Fix checkpatch.pl errors
>     - remove mips64 ifdef
>     - changed bitfield usage to extract32
>     - squashed register addition patch into this one
> 
>  target/mips/cpu.h       |  1 +
>  target/mips/translate.c | 71 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 

Hi, Craig,

So far it is going well. You respond well to the reviews. I encourage you to 
persevere, this is a nice addition to QEMU's MIPS functionality, we want it.

Just something about overall patch organization:

This (1/7) patch should be split/refactored in this way:

PATCH 1: Introduce MXU registers (16 fields and their names, plus 
initialization)

PATCH 2: Add MXU opcodes (if possible, add ALL MXU opcodes, there are 50 to 60 
ones, if I am not mistaken)

PATCH 3: The rest of this patch (so, S32I2M and S32M2I emulation)

... and further patches as you already organized.

However, there is more. We plan to significantly revamp translate.c in near 
future, and this series, even though it is consistent with the rest of code in 
the same file, would not fit in its present state well into the new 
organization of translate.c. I am asking you, therefore, to reorganize code in 
the following fashion:

- implementation of emulation of each instruction should be in a separate 
function (baring some exceptions); for this patch functions gen_mxu_s32i2m() 
and gen_mxu_s32m2i() should be created; the only arguments of these function 
should be "DisasContext *ctx" and "uint32_t opc";

- each such function should be preceded by a comment similar to this:

/*
 * S32M2I XRa, rb - Register move from XRF to GRF
 */

- gen_mxu() would disappear;

- decode_opc_special2_legacy() should call various gen_mxu_XXX() directly, in 
separate "case" sections for each instruction.

Please rearrange the code this way.

Thanks,
Aleksandar

> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 009202cf64..4b2948a2c8 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -170,6 +170,7 @@ struct TCState {
>          MSACSR_FS_MASK)
> 
>      float_status msa_fp_status;
> +    target_ulong mxu_gpr[16];
>  };
> 
>  typedef struct CPUMIPSState CPUMIPSState;
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index bdd880bb77..ef819d67e0 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -364,6 +364,9 @@ enum {
>      OPC_CLO      = 0x21 | OPC_SPECIAL2,
>      OPC_DCLZ     = 0x24 | OPC_SPECIAL2,
>      OPC_DCLO     = 0x25 | OPC_SPECIAL2,
> +    /* MXU */
> +    OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
> +    OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
>      /* Special */
>      OPC_SDBBP    = 0x3F | OPC_SPECIAL2,
>  };
> @@ -1398,6 +1401,9 @@ static TCGv_i32 fpu_fcr0, fpu_fcr31;
>  static TCGv_i64 fpu_f64[32];
>  static TCGv_i64 msa_wr_d[64];
> 
> +/* MXU registers */
> +static TCGv mxu_gpr[16];
> +
>  #include "exec/gen-icount.h"
> 
>  #define gen_helper_0e0i(name, arg) do {                           \
> @@ -1517,6 +1523,13 @@ static const char * const msaregnames[] = {
>      "w30.d0", "w30.d1", "w31.d0", "w31.d1",
>  };
> 
> +static const char * const mxuregnames[] = {
> +    "XR1",  "XR2",  "XR3",  "XR4",  "XR5",
> +    "XR6",  "XR7",  "XR8",  "XR9",  "XR10",
> +    "XR11", "XR12", "XR13", "XR14", "XR15",
> +    "XR16",
> +};
> +
>  #define LOG_DISAS(...)                                                       
>  \
>      do {                                                                     
>  \
>          if (MIPS_DEBUG_DISAS) {                                              
>  \
> @@ -1550,6 +1563,23 @@ static inline void gen_store_gpr (TCGv t, int reg)
>          tcg_gen_mov_tl(cpu_gpr[reg], t);
>  }
> 
> +/* MXU General purpose registers moves. */
> +static inline void gen_load_mxu_gpr(TCGv t, int reg)
> +{
> +    if (reg == 0) {
> +        tcg_gen_movi_tl(t, 0);
> +    } else {
> +        tcg_gen_mov_tl(t, mxu_gpr[reg - 1]);
> +    }
> +}
> +
> +static inline void gen_store_mxu_gpr(TCGv t, int reg)
> +{
> +    if (reg != 0) {
> +        tcg_gen_mov_tl(mxu_gpr[reg - 1], t);
> +    }
> +}
> +
>  /* Moves to/from shadow registers. */
>  static inline void gen_load_srsgpr (int from, int to)
>  {
> @@ -3738,6 +3768,35 @@ static void gen_cl (DisasContext *ctx, uint32_t opc,
>      }
>  }
> 
> +/* MXU Instructions */
> +static void gen_mxu(DisasContext *ctx, uint32_t opc)
> +{
> +    TCGv t0;
> +    uint32_t xra, rb;
> +
> +    t0 = tcg_temp_new();
> +
> +    switch (opc) {
> +    case OPC_MXU_S32I2M:
> +        xra = extract32(ctx->opcode, 6, 5);
> +        rb = extract32(ctx->opcode, 16, 5);
> +
> +        gen_load_gpr(t0, rb);
> +        gen_store_mxu_gpr(t0, xra);
> +        break;
> +
> +    case OPC_MXU_S32M2I:
> +        xra = extract32(ctx->opcode, 6, 5);
> +        rb = extract32(ctx->opcode, 16, 5);
> +
> +        gen_load_mxu_gpr(t0, xra);
> +        gen_store_gpr(t0, rb);
> +        break;
> +    }
> +
> +    tcg_temp_free(t0);
> +}
> +
>  /* Godson integer instructions */
>  static void gen_loongson_integer(DisasContext *ctx, uint32_t opc,
>                                   int rd, int rs, int rt)
> @@ -17818,6 +17877,12 @@ static void decode_opc_special2_legacy(CPUMIPSState 
> *env, DisasContext *ctx)
>          check_insn(ctx, INSN_LOONGSON2F);
>          gen_loongson_integer(ctx, op1, rd, rs, rt);
>          break;
> +
> +    case OPC_MXU_S32I2M:
> +    case OPC_MXU_S32M2I:
> +        gen_mxu(ctx, op1);
> +        break;
> +
>      case OPC_CLO:
>      case OPC_CLZ:
>          check_insn(ctx, ISA_MIPS32);
> @@ -20742,6 +20807,12 @@ void mips_tcg_init(void)
>      fpu_fcr31 = tcg_global_mem_new_i32(cpu_env,
>                                         offsetof(CPUMIPSState, 
> active_fpu.fcr31),
>                                         "fcr31");
> +
> +    for (i = 0; i < 16; i++)
> +        mxu_gpr[i] = tcg_global_mem_new(cpu_env,
> +                                        offsetof(CPUMIPSState,
> +                                                 active_tc.mxu_gpr[i]),
> +                                        mxuregnames[i]);
>  }
> 
>  #include "translate_init.inc.c"
> --
> 2.18.0
> 
>