Re: [Qemu-devel] [PATCH 2/9] target/mips: Introduce 32 R5900 128-bit multimedia registers

[Aleksandar Markovic]

> From: Fredrik Noring <address@hidden>
> Sent: Sunday, January 13, 2019 8:03 PM
> To: Aleksandar Markovic; Aurelien Jarno; Philippe Mathieu-Daudé
> Cc: Jürgen Urban; Maciej W. Rozycki; address@hidden
> Subject: [PATCH 2/9] target/mips: Introduce 32 R5900 128-bit multimedia 
> registers
> 
> The 32 R5900 128-bit MMRs are split into two 64-bit halves: the lower
> halves are the GPRs and the upper halves are accessible by the R5900-
> specific multimedia instructions.
> 
> Signed-off-by: Fredrik Noring <address@hidden>
> ---
>  target/mips/cpu.h       |  2 ++
>  target/mips/translate.c | 14 ++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 03c03fd8c6..9ff4a68d90 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -180,6 +180,8 @@ struct TCState {
>  #define MXU_CR_RD_EN    1
>  #define MXU_CR_MXU_EN   0
> 
> +    /* Upper 64-bit MMRs (multimedia registers); the lower 64-bit are GPRs */
> +    uint64_t mmr[32];
>  };
> 
>  typedef struct CPUMIPSState CPUMIPSState;
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index a538351032..9d5150ec8b 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -2455,7 +2455,10 @@ static TCGv_i32 fpu_fcr0, fpu_fcr31;
>  static TCGv_i64 fpu_f64[32];
>  static TCGv_i64 msa_wr_d[64];
> 
> -#if !defined(TARGET_MIPS64)
> +#if defined(TARGET_MIPS64)
> +/* Upper 64-bit MMRs (multimedia registers); the lower 64-bit are GPRs */
> +static TCGv_i64 cpu_mmr[32];
> +#else

Naming is perfect, but:

#if-#else-#endif is confusing here:
- MMR declaration should be under it own #if defined(TARGET_MIPS64)
- MXU declaration should be under separate #if !defined(TARGET_MIPS64)
(we are not dealing with two variants (64/32) of the same item, but with two
separate items, one is valid for 64-bit only, another for 32-bit only)

>  /* MXU registers */
>  static TCGv mxu_gpr[NUMBER_OF_MXU_REGISTERS - 1];
>  static TCGv mxu_CR;
> @@ -29785,7 +29788,14 @@ void mips_tcg_init(void)
>      fpu_fcr31 = tcg_global_mem_new_i32(cpu_env,
>                                         offsetof(CPUMIPSState, 
> active_fpu.fcr31),
>                                         "fcr31");
> -#if !defined(TARGET_MIPS64)
> +#if defined(TARGET_MIPS64)
> +    cpu_mmr[0] = NULL;
> +    for (i = 1; i < 32; i++)
> +        cpu_mmr[i] = tcg_global_mem_new_i64(cpu_env,
> +                                            offsetof(CPUMIPSState,
> +                                                     active_tc.mmr[i]),
> +                                            regnames[i]);
> +#else
>      for (i = 0; i < NUMBER_OF_MXU_REGISTERS - 1; i++) {
>          mxu_gpr[i] = tcg_global_mem_new(cpu_env,
>                                          offsetof(CPUMIPSState,
> --

The same applies here.

> 2.19.2