| Message ID | fa2941ee17c35fba2512ab530bde3a1807d5769e.1541174426.git.noring@nocrew.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 | expand |
On 2/11/18 17:08, Fredrik Noring wrote: > MFLO1, MFHI1, MTLO1 and MTHI1 are generated in gen_HILO1_tx79 instead of > the generic gen_HILO. > Aleksandar, if you are OK with this patch, can you add: Fixes: 8d927f7cb4b > Signed-off-by: Fredrik Noring <noring@nocrew.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > target/mips/translate.c | 67 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 56 insertions(+), 11 deletions(-) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 60320cbe69..f3993cf7d7 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -4359,24 +4359,72 @@ static void gen_shift(DisasContext *ctx, uint32_t opc, > tcg_temp_free(t1); > } > > +/* Move to and from TX79 HI1/LO1 registers. */ > +static void gen_HILO1_tx79(DisasContext *ctx, uint32_t opc, int reg) > +{ > + if (reg == 0 && (opc == TX79_MMI_MFHI1 || opc == TX79_MMI_MFLO1)) { > + /* Treat as NOP. */ > + return; > + } > + > + switch (opc) { > + case TX79_MMI_MFHI1: > +#if defined(TARGET_MIPS64) > + tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]); > +#else > + tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]); > +#endif > + break; > + case TX79_MMI_MFLO1: > +#if defined(TARGET_MIPS64) > + tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[1]); > +#else > + tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[1]); > +#endif > + break; > + case TX79_MMI_MTHI1: > + if (reg != 0) { > +#if defined(TARGET_MIPS64) > + tcg_gen_ext32s_tl(cpu_HI[1], cpu_gpr[reg]); > +#else > + tcg_gen_mov_tl(cpu_HI[1], cpu_gpr[reg]); > +#endif > + } else { > + tcg_gen_movi_tl(cpu_HI[1], 0); > + } > + break; > + case TX79_MMI_MTLO1: > + if (reg != 0) { > +#if defined(TARGET_MIPS64) > + tcg_gen_ext32s_tl(cpu_LO[1], cpu_gpr[reg]); > +#else > + tcg_gen_mov_tl(cpu_LO[1], cpu_gpr[reg]); > +#endif > + } else { > + tcg_gen_movi_tl(cpu_LO[1], 0); > + } > + break; > + default: > + MIPS_INVAL("MFTHILO TX79"); > + generate_exception_end(ctx, EXCP_RI); > + break; > + } > +} > + > /* Arithmetic on HI/LO registers */ > static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) > { > - if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 || > - opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) { > + if (reg == 0 && (opc == OPC_MFHI || opc == OPC_MFLO)) { > /* Treat as NOP. */ > return; > } > > if (acc != 0) { > - if (!(ctx->insn_flags & INSN_R5900)) { > - check_dsp(ctx); > - } > + check_dsp(ctx); > } > > switch (opc) { > case OPC_MFHI: > - case TX79_MMI_MFHI1: > #if defined(TARGET_MIPS64) > if (acc != 0) { > tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]); > @@ -4387,7 +4435,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) > } > break; > case OPC_MFLO: > - case TX79_MMI_MFLO1: > #if defined(TARGET_MIPS64) > if (acc != 0) { > tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]); > @@ -4398,7 +4445,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) > } > break; > case OPC_MTHI: > - case TX79_MMI_MTHI1: > if (reg != 0) { > #if defined(TARGET_MIPS64) > if (acc != 0) { > @@ -4413,7 +4459,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) > } > break; > case OPC_MTLO: > - case TX79_MMI_MTLO1: > if (reg != 0) { > #if defined(TARGET_MIPS64) > if (acc != 0) { > @@ -26500,11 +26545,11 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx) > break; > case TX79_MMI_MTLO1: > case TX79_MMI_MTHI1: > - gen_HILO(ctx, opc, 1, rs); > + gen_HILO1_tx79(ctx, opc, rs); > break; > case TX79_MMI_MFLO1: > case TX79_MMI_MFHI1: > - gen_HILO(ctx, opc, 1, rd); > + gen_HILO1_tx79(ctx, opc, rd); > break; > case TX79_MMI_MADD: /* TODO: TX79_MMI_MADD */ > case TX79_MMI_MADDU: /* TODO: TX79_MMI_MADDU */ >
On 11/2/18 4:08 PM, Fredrik Noring wrote: > + switch (opc) { > + case TX79_MMI_MFHI1: > +#if defined(TARGET_MIPS64) > + tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]); > +#else > + tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]); > +#endif You do not need this ifdef. This is already done in tcg/tcg-op.h: $ grep tcg_gen_ext32s_tl tcg/tcg-op.h #define tcg_gen_ext32s_tl tcg_gen_ext32s_i64 #define tcg_gen_ext32s_tl tcg_gen_mov_i32 r~
Thank you for your reviews, Philippe and Richard, > > + switch (opc) { > > + case TX79_MMI_MFHI1: > > +#if defined(TARGET_MIPS64) > > + tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]); > > +#else > > + tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]); > > +#endif > > You do not need this ifdef. This is already done in tcg/tcg-op.h: > > $ grep tcg_gen_ext32s_tl tcg/tcg-op.h > #define tcg_gen_ext32s_tl tcg_gen_ext32s_i64 > #define tcg_gen_ext32s_tl tcg_gen_mov_i32 It appears the correct function is tcg_gen_mov_tl because the TX79 manual says MFHI: GPR[rd]63..0 <- HI63..0 MFLO: GPR[rd]63..0 <- LO63..0 MTHI: HI63..0 <- GPR[rs]63..0 MTLO: LO63..0 <- GPR[rs]63..0 MFHI1: GPR[rd]63..0 <- HI127..64 MFLO1: GPR[rd]63..0 <- LO127..64 MTHI1: HI127..64 <- GPR[rs]63..0 MTLO1: LO127..64 <- GPR[rs]63..0 so the GPR is copied to/from in full in all cases. This is slightly different to how acc = 1 is handled in gen_HILO. Fredrik
On Sun, 4 Nov 2018, Fredrik Noring wrote: > It appears the correct function is tcg_gen_mov_tl because the TX79 manual > says > > MFHI: GPR[rd]63..0 <- HI63..0 > MFLO: GPR[rd]63..0 <- LO63..0 > MTHI: HI63..0 <- GPR[rs]63..0 > MTLO: LO63..0 <- GPR[rs]63..0 > MFHI1: GPR[rd]63..0 <- HI127..64 > MFLO1: GPR[rd]63..0 <- LO127..64 > MTHI1: HI127..64 <- GPR[rs]63..0 > MTLO1: LO127..64 <- GPR[rs]63..0 > > so the GPR is copied to/from in full in all cases. This is slightly > different to how acc = 1 is handled in gen_HILO. However `gen_HILO' looks wrong to me as it'll truncate the values of $acc3-$acc1 with the 64-bit DSP ASE. Maciej
Thanks for checking this, Maciej, [ Cc-ing Jia Liu, who added MIPS ASE DSP support in commit 4133498f8e532f "Use correct acc value to index cpu_HI/cpu_LO rather than using a fix number", in case there are known ISA deviations. ] > However `gen_HILO' looks wrong to me as it'll truncate the values of > $acc3-$acc1 with the 64-bit DSP ASE. I can post a patch to fix gen_HILO. The best reference I have found so far is "MIPS Architecture for Programmers Volume IV-e: MIPS DSP Module for microMIPS64 Architecture" https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00765-2B-microMIPS64DSP-AFP-03.02.pdf that says MFHI: GPR[rds]63..0 <- HI[ac]63..0 MFLO: GPR[rdt]63..0 <- LO[ac]63..0 MTHI: HI[ac]63..0 <- GPR[rs]63..0 MTLO: LO[ac]63..0 <- GPR[rs]63..0 where ac can range from 0 to 3. Do you have a link to a better reference, by chance, that isn't tied to microMIPS? Fredrik
On Mon, 5 Nov 2018, Fredrik Noring wrote: > where ac can range from 0 to 3. Do you have a link to a better reference, > by chance, that isn't tied to microMIPS? Here: <https://www.mips.com/downloads/> you'll find everything you need I believe. HTH, Maciej
diff --git a/target/mips/translate.c b/target/mips/translate.c index 60320cbe69..f3993cf7d7 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -4359,24 +4359,72 @@ static void gen_shift(DisasContext *ctx, uint32_t opc, tcg_temp_free(t1); } +/* Move to and from TX79 HI1/LO1 registers. */ +static void gen_HILO1_tx79(DisasContext *ctx, uint32_t opc, int reg) +{ + if (reg == 0 && (opc == TX79_MMI_MFHI1 || opc == TX79_MMI_MFLO1)) { + /* Treat as NOP. */ + return; + } + + switch (opc) { + case TX79_MMI_MFHI1: +#if defined(TARGET_MIPS64) + tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]); +#else + tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]); +#endif + break; + case TX79_MMI_MFLO1: +#if defined(TARGET_MIPS64) + tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[1]); +#else + tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[1]); +#endif + break; + case TX79_MMI_MTHI1: + if (reg != 0) { +#if defined(TARGET_MIPS64) + tcg_gen_ext32s_tl(cpu_HI[1], cpu_gpr[reg]); +#else + tcg_gen_mov_tl(cpu_HI[1], cpu_gpr[reg]); +#endif + } else { + tcg_gen_movi_tl(cpu_HI[1], 0); + } + break; + case TX79_MMI_MTLO1: + if (reg != 0) { +#if defined(TARGET_MIPS64) + tcg_gen_ext32s_tl(cpu_LO[1], cpu_gpr[reg]); +#else + tcg_gen_mov_tl(cpu_LO[1], cpu_gpr[reg]); +#endif + } else { + tcg_gen_movi_tl(cpu_LO[1], 0); + } + break; + default: + MIPS_INVAL("MFTHILO TX79"); + generate_exception_end(ctx, EXCP_RI); + break; + } +} + /* Arithmetic on HI/LO registers */ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) { - if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 || - opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) { + if (reg == 0 && (opc == OPC_MFHI || opc == OPC_MFLO)) { /* Treat as NOP. */ return; } if (acc != 0) { - if (!(ctx->insn_flags & INSN_R5900)) { - check_dsp(ctx); - } + check_dsp(ctx); } switch (opc) { case OPC_MFHI: - case TX79_MMI_MFHI1: #if defined(TARGET_MIPS64) if (acc != 0) { tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]); @@ -4387,7 +4435,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) } break; case OPC_MFLO: - case TX79_MMI_MFLO1: #if defined(TARGET_MIPS64) if (acc != 0) { tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]); @@ -4398,7 +4445,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) } break; case OPC_MTHI: - case TX79_MMI_MTHI1: if (reg != 0) { #if defined(TARGET_MIPS64) if (acc != 0) { @@ -4413,7 +4459,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg) } break; case OPC_MTLO: - case TX79_MMI_MTLO1: if (reg != 0) { #if defined(TARGET_MIPS64) if (acc != 0) { @@ -26500,11 +26545,11 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx) break; case TX79_MMI_MTLO1: case TX79_MMI_MTHI1: - gen_HILO(ctx, opc, 1, rs); + gen_HILO1_tx79(ctx, opc, rs); break; case TX79_MMI_MFLO1: case TX79_MMI_MFHI1: - gen_HILO(ctx, opc, 1, rd); + gen_HILO1_tx79(ctx, opc, rd); break; case TX79_MMI_MADD: /* TODO: TX79_MMI_MADD */ case TX79_MMI_MADDU: /* TODO: TX79_MMI_MADDU */
MFLO1, MFHI1, MTLO1 and MTHI1 are generated in gen_HILO1_tx79 instead of the generic gen_HILO. Signed-off-by: Fredrik Noring <noring@nocrew.org> --- target/mips/translate.c | 67 ++++++++++++++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 11 deletions(-)