[Qemu-devel] Re: [4545] Switch MIPS movf/movt to TCG.

[Richard Sandiford <rdsandiford@googlemail.com>]

Hi Thiemo,

Thiemo Seufer <ths@networkno.de> writes:
> Modified: trunk/target-mips/translate.c
> ===================================================================
> --- trunk/target-mips/translate.c	2008-05-23 17:33:39 UTC (rev 4544)
> +++ trunk/target-mips/translate.c	2008-05-23 18:06:27 UTC (rev 4545)
> @@ -5450,19 +5450,33 @@
>  
>  static void gen_movci (DisasContext *ctx, int rd, int rs, int cc, int tf)
>  {
> +    TCGv r_ptr = tcg_temp_new(TCG_TYPE_PTR);
> +    TCGv r_tmp = new_tmp();
> +    TCGv t0 = tcg_temp_new(TCG_TYPE_TL);
> +    TCGv t1 = tcg_temp_new(TCG_TYPE_TL);
> +    int l1 = gen_new_label();
>      uint32_t ccbit;
> +    TCGCond cond;
>  
> -    gen_load_gpr(cpu_T[0], rd);
> -    gen_load_gpr(cpu_T[1], rs);
> -    if (cc) {
> +    if (cc)
>          ccbit = 1 << (24 + cc);
> -    } else
> +    else
>          ccbit = 1 << 23;
> -    if (!tf)
> -        gen_op_movf(ccbit);
> +    if (tf)
> +        cond = TCG_COND_NE;
>      else
> -        gen_op_movt(ccbit);
> -    gen_store_gpr(cpu_T[0], rd);
> +        cond = TCG_COND_EQ;
> +
> +    gen_load_gpr(t0, rd);
> +    gen_load_gpr(t1, rs);
> +    tcg_gen_ld_ptr(r_ptr, cpu_env, offsetof(CPUState, fpu));
> +    tcg_gen_ld_i32(r_tmp, r_ptr, offsetof(CPUMIPSFPUContext, fcr31));
> +    tcg_gen_andi_i32(r_tmp, r_tmp, ccbit);
> +    tcg_gen_brcond_i32(cond, r_tmp, tcg_const_i32(0), l1);
> +    tcg_gen_mov_tl(t0, t1);
> +    gen_set_label(l1);
> +    dead_tmp(r_tmp);
> +    gen_store_gpr(t0, rd);
>  }
>  
>  #define GEN_MOVCF(fmt)                                                \

Looks like there's a couple of problems here:

  - T0 and T1 outlive the first basic block, so the GPR loads get
    deleted as dead.

  - This a branch-over-move, so the condition is reversed.
    It should be TCG_COND_EQ (to zero) for MOVT and
    TCG_COND_NE (to zero) for MOVF.

The patch below seems to work for me.

(Style elsewhere in translate.c seemed to be to use C blocks to
emphasise the lifetimes of temporaries, so I did the same here.
Hope that's OK.)

Richard

Index: qemu/target-mips/translate.c
===================================================================
--- qemu.orig/target-mips/translate.c	2008-05-24 11:12:56.000000000 +0100
+++ qemu/target-mips/translate.c	2008-05-24 11:45:16.000000000 +0100
@@ -5664,10 +5664,6 @@ static void gen_cp1 (DisasContext *ctx, 
 
 static void gen_movci (DisasContext *ctx, int rd, int rs, int cc, int tf)
 {
-    TCGv r_ptr = tcg_temp_new(TCG_TYPE_PTR);
-    TCGv r_tmp = new_tmp();
-    TCGv t0 = tcg_temp_new(TCG_TYPE_TL);
-    TCGv t1 = tcg_temp_new(TCG_TYPE_TL);
     int l1 = gen_new_label();
     uint32_t ccbit;
     TCGCond cond;
@@ -5677,20 +5673,26 @@ static void gen_movci (DisasContext *ctx
     else
         ccbit = 1 << 23;
     if (tf)
-        cond = TCG_COND_NE;
-    else
         cond = TCG_COND_EQ;
+    else
+        cond = TCG_COND_NE;
+
+    gen_load_gpr(cpu_T[0], rd);
+    gen_load_gpr(cpu_T[1], rs);
+    {
+        TCGv r_ptr = tcg_temp_new(TCG_TYPE_PTR);
+        TCGv r_tmp = new_tmp();
+
+        tcg_gen_ld_ptr(r_ptr, cpu_env, offsetof(CPUState, fpu));
+        tcg_gen_ld_i32(r_tmp, r_ptr, offsetof(CPUMIPSFPUContext, fcr31));
+        tcg_gen_andi_i32(r_tmp, r_tmp, ccbit);
+        tcg_gen_brcondi_i32(cond, r_tmp, 0, l1);
+        dead_tmp(r_tmp);
+    }
+    tcg_gen_mov_tl(cpu_T[0], cpu_T[1]);
 
-    gen_load_gpr(t0, rd);
-    gen_load_gpr(t1, rs);
-    tcg_gen_ld_ptr(r_ptr, cpu_env, offsetof(CPUState, fpu));
-    tcg_gen_ld_i32(r_tmp, r_ptr, offsetof(CPUMIPSFPUContext, fcr31));
-    tcg_gen_andi_i32(r_tmp, r_tmp, ccbit);
-    tcg_gen_brcondi_i32(cond, r_tmp, 0, l1);
-    tcg_gen_mov_tl(t0, t1);
     gen_set_label(l1);
-    dead_tmp(r_tmp);
-    gen_store_gpr(t0, rd);
+    gen_store_gpr(cpu_T[0], rd);
 }
 
 #define GEN_MOVCF(fmt)                                                \