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

[Aleksandar Markovic]

> From: Aleksandar Markovic
> Sent: Tuesday, January 15, 2019 10:20 PM
> To: Fredrik Noring; Aurelien Jarno; Philippe Mathieu-Daudé
> Cc: Jürgen Urban; Maciej W. Rozycki; address@hidden
> Subject: Re: [PATCH 2/9] target/mips: Introduce 32 R5900 128-bit multimedia 
> registers
> 
> > 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>
> > ---

Since this patch is not a subject of any disagreement, and "#ifdef"
structure is of a cosmetic nature, I will do these minor cosmetic
changes while applying, and insert this patch in the next MIPS pull
request, scheduled shortly.

Reviewed-by: Aleksandar Markovic <address@hidden>

Other patches from this series will not be in the pull request.

> >  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
> 
>