target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

MFLO1, MFHI1, MTLO1 and MTHI1 are generated in gen_HILO1_tx79 instead of the generic gen_HILO.

Signed-off-by Fredrik Noring <[email protected]>

---
  target/mips/translate.c | 67 ++++++++++++++++++++++++++++++++++-------
  1 file changed, 56 insertions(+), 11 deletions(-)

target/mips/translate.c +56 -11

index 60320cbe69..f3993cf7d7 100644
@@ -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 */

target/mips/translate.c static void gen_shift(DisasContext *ctx, uint32_t opc, · line 4376

Quoted 7 lines

target/mips/translate.c +7 -0

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

However `gen_HILO' looks wrong to me as it'll truncate the values of
$acc3-$acc1 with the 64-bit DSP ASE.

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?

DIV1 and DIVU1 are generated in gen_div1_tx79 instead of the generic gen_muldiv.

Signed-off-by Fredrik Noring <[email protected]>

---
  target/mips/translate.c | 65 +++++++++++++++++++++++++++++++++++++----
  1 file changed, 59 insertions(+), 6 deletions(-)

target/mips/translate.c +59 -6

index f3993cf7d7..6e5a8a2565 100644
@@ -4759,6 +4759,63 @@ static void gen_r6_muldiv(DisasContext *ctx, int opc, int rd, int rs, int rt)
      tcg_temp_free(t1);
  }
 
+static void gen_div1_tx79(DisasContext *ctx, uint32_t opc, int rs, int rt)
+{
+    TCGv t0, t1;
+
+    t0 = tcg_temp_new();
+    t1 = tcg_temp_new();
+
+    gen_load_gpr(t0, rs);
+    gen_load_gpr(t1, rt);
+
+    switch (opc) {
+    case TX79_MMI_DIV1:
+        {
+            TCGv t2 = tcg_temp_new();
+            TCGv t3 = tcg_temp_new();
+            tcg_gen_ext32s_tl(t0, t0);
+            tcg_gen_ext32s_tl(t1, t1);
+            tcg_gen_setcondi_tl(TCG_COND_EQ, t2, t0, INT_MIN);
+            tcg_gen_setcondi_tl(TCG_COND_EQ, t3, t1, -1);
+            tcg_gen_and_tl(t2, t2, t3);
+            tcg_gen_setcondi_tl(TCG_COND_EQ, t3, t1, 0);
+            tcg_gen_or_tl(t2, t2, t3);
+            tcg_gen_movi_tl(t3, 0);
+            tcg_gen_movcond_tl(TCG_COND_NE, t1, t2, t3, t2, t1);
+            tcg_gen_div_tl(cpu_LO[1], t0, t1);
+            tcg_gen_rem_tl(cpu_HI[1], t0, t1);
+            tcg_gen_ext32s_tl(cpu_LO[1], cpu_LO[1]);
+            tcg_gen_ext32s_tl(cpu_HI[1], cpu_HI[1]);
+            tcg_temp_free(t3);
+            tcg_temp_free(t2);
+        }
+        break;
+    case TX79_MMI_DIVU1:
+        {
+            TCGv t2 = tcg_const_tl(0);
+            TCGv t3 = tcg_const_tl(1);
+            tcg_gen_ext32u_tl(t0, t0);
+            tcg_gen_ext32u_tl(t1, t1);
+            tcg_gen_movcond_tl(TCG_COND_EQ, t1, t1, t2, t3, t1);
+            tcg_gen_divu_tl(cpu_LO[1], t0, t1);
+            tcg_gen_remu_tl(cpu_HI[1], t0, t1);
+            tcg_gen_ext32s_tl(cpu_LO[1], cpu_LO[1]);
+            tcg_gen_ext32s_tl(cpu_HI[1], cpu_HI[1]);
+            tcg_temp_free(t3);
+            tcg_temp_free(t2);
+        }
+        break;
+    default:
+        MIPS_INVAL("div1 TX79");
+        generate_exception_end(ctx, EXCP_RI);
+        goto out;
+    }
+ out:
+    tcg_temp_free(t0);
+    tcg_temp_free(t1);
+}
+
  static void gen_muldiv(DisasContext *ctx, uint32_t opc,
                         int acc, int rs, int rt)
  {

@@ -4771,14 +4828,11 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
      gen_load_gpr(t1, rt);
 
      if (acc != 0) {
-        if (!(ctx->insn_flags & INSN_R5900)) {
-            check_dsp(ctx);
-        }
+        check_dsp(ctx);
      }
 
      switch (opc) {
      case OPC_DIV:
-    case TX79_MMI_DIV1:
          {
              TCGv t2 = tcg_temp_new();
              TCGv t3 = tcg_temp_new();

@@ -4800,7 +4854,6 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
          }
          break;
      case OPC_DIVU:
-    case TX79_MMI_DIVU1:
          {
              TCGv t2 = tcg_const_tl(0);
              TCGv t3 = tcg_const_tl(1);

@@ -26541,7 +26594,7 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
          break;
      case TX79_MMI_DIV1:
      case TX79_MMI_DIVU1:
-        gen_muldiv(ctx, opc, 1, rs, rt);
+        gen_div1_tx79(ctx, opc, rs, rt);
          break;
      case TX79_MMI_MTLO1:
      case TX79_MMI_MTHI1:

From: Fredrik Noring <[email protected]>

Subject: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F,T}{HI,LO}1 and DIV[U]1

This series amends the R5900 support with the following changes:

...

For LL, SC, LLD and SCD instructions, there is a need to properly insulate their R5900 versions too, similar to this:

    case OPC_SC:
        if(ctx->insn_flags & INSN_R5900) {
             check_insn_opc_user_only(ctx, INSN_R5900);
        } else {
            check_insn(ctx, ISA_MIPS2);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

(the code above is just a form of pseudocode illustrating the idea; I don't guarantee the correctness for build purposes, or if this is the best code organization)

Non-R5900 code (for the time being) should never invoke check_insn_opc_user_only(). *The only way* of distinguishing R5900 code paths from the other CPUs code paths should be by using "if(ctx->insn_flags & INSN_R5900)"!

For changes in decode_opc_special_legacy(), there shouldn't be there, but there should be a separate function decode_opc_special_tx59() or so.

From: Fredrik Noring <[email protected]>

Subject: Re: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F,T}{HI,LO}1 and DIV[> U]1

Thank you for your review, Aleksandar,

Quoted 11 lines

For LL, SC, LLD and SCD instructions, there is a need to properly insulate their R5900 versions too, similar to this:

    case OPC_SC:
        if(ctx->insn_flags & INSN_R5900) {
             check_insn_opc_user_only(ctx, INSN_R5900);
        } else {
            check_insn(ctx, ISA_MIPS2);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

Would you accept the simplification to omit the else clause? Like this:

    case OPC_SC:
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

The code will, of course, expand into a double-check of INSN_R5900:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            if (unlikely(ctx->insn_flags & INSN_R5900)) {
                generate_exception_end(ctx, EXCP_RI);
            }
#endif
        }

Quoted 8 lines

(the code above is just a form of pseudocode illustrating the idea; I don't guarantee the correctness for build purposes, or if this is the best code organization)

Non-R5900 code (for the time being) should never invoke check_insn_opc_user_only(). *The only way* of distinguishing R5900 code paths from the other CPUs code paths should be by using "if(ctx->insn_flags & INSN_R5900)"!

OK.

Quoted 2 lines

For changes in decode_opc_special_legacy(), there shouldn't be there, but there should be a separate function decode_opc_special_tx59() or so.

Sure, I will copy the 82 line function then...

..., and patch the following:

target/mips/translate.c +1 -3

@@ -23904,7 +23904,7 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext > *ctx)
     case OPC_MOVN:         /* Conditional move */
     case OPC_MOVZ:
         check_insn(ctx, ISA_MIPS4 | ISA_MIPS32 |
-                   INSN_LOONGSON2E | INSN_LOONGSON2F | INSN_R5900);
+                   INSN_LOONGSON2E | INSN_LOONGSON2F);
         gen_cond_move(ctx, op1, rd, rs, rt);
         break;
     case OPC_MFHI:          /* Move from HI/LO */

@@ -23931,8 +23931,6 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext > *ctx)
             check_insn(ctx, INSN_VR54XX);
             op1 = MASK_MUL_VR54XX(ctx->opcode);
             gen_mul_vr54xx(ctx, op1, rd, rs, rt);
-        } else if (ctx->insn_flags & INSN_R5900) {
-            gen_mul_txx9(ctx, op1, rd, rs, rt);
         } else {
             gen_muldiv(ctx, op1, rd & 3, rs, rt);
         }

Fredrik

For LL, SC, LLD and SCD instructions, there is a need to properly insulate their R5900 versions too, similar to this:

    case OPC_SC:
        if(ctx->insn_flags & INSN_R5900) {
             check_insn_opc_user_only(ctx, INSN_R5900);
        } else {
            check_insn(ctx, ISA_MIPS2);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

(the code above is just a form of pseudocode illustrating the idea; I don't guarantee the correctness for build purposes, or if this is the best code organization)

Non-R5900 code (for the time being) should never invoke check_insn_opc_user_only(). *The only way* of distinguishing R5900 code paths from the other CPUs code paths should be by using "if(ctx->insn_flags & INSN_R5900)"!

For changes in decode_opc_special_legacy(), there shouldn't be there, but there should be a separate function decode_opc_special_tx59() or so.

I think the following code would be even better:

    case OPC_SC:
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

I don't mind. Later on, we can drop the second argument of check_insn_opc_user_only() altogether, and the code would expand to the minimal and clear:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            generate_exception_end(ctx, EXCP_RI);
#endif
        }

but at this moment this is not a source of concern to me at all.

No, no, you don't need to copy 82 lines. First, you can assume that you are already in INSN_R5900 case - no need for frequent check_insn()s. Further, you can omit everything that is not needed for R5900 (for example, entire OPC_MOVCI case).