Re: [Qemu-devel] [PATCH 11/14] ppc: store CR registers in 32 1-bit registers

[Paolo Bonzini <pbonzini@redhat.com>]

Il 18/09/2014 22:25, Tom Musta ha scritto:
> This breaks what you did in patch 5, which used LE bit numbering to
> perform shifts.

Yeah, I change "1 << x" to "8 >> x" in this patch for the fcmp
helpers, but not the others.

> And it breaks other code that uses the old LE
> convention.

I'll fix it like this:

git diff target-ppc/fpu_helper.c
diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index da93d12..06e4559 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -1018,32 +1018,32 @@ uint32_t helper_ftdiv(uint64_t fra, uint64_t frb)
     if (unlikely(float64_is_infinity(fra) ||
                  float64_is_infinity(frb) ||
                  float64_is_zero(frb))) {
-        fe_flag = 1;
-        fg_flag = 1;
+        fe_flag = 8 >> CRF_EQ;
+        fg_flag = 8 >> CRF_GT;
     } else {
         int e_a = ppc_float64_get_unbiased_exp(fra);
         int e_b = ppc_float64_get_unbiased_exp(frb);
 
         if (unlikely(float64_is_any_nan(fra) ||
                      float64_is_any_nan(frb))) {
-            fe_flag = 1;
+            fe_flag = 8 >> CRF_EQ;
         } else if ((e_b <= -1022) || (e_b >= 1021)) {
-            fe_flag = 1;
+            fe_flag = 8 >> CRF_EQ;
         } else if (!float64_is_zero(fra) &&
                    (((e_a - e_b) >= 1023) ||
                     ((e_a - e_b) <= -1021) ||
                     (e_a <= -970))) {
-            fe_flag = 1;
+            fe_flag = 8 >> CRF_EQ;
         }
 
         if (unlikely(float64_is_zero_or_denormal(frb))) {
             /* XB is not zero because of the above check and */
             /* so must be denormalized.                      */
-            fg_flag = 1;
+            fg_flag = 8 >> CRF_GT;
         }
     }
 
-    return 0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0);
+    return (8 >> CRF_LT) | fg_flag | fe_flag;
 }
 
 uint32_t helper_ftsqrt(uint64_t frb)

and similarly for ftsqrt.

Paolo

> There are some other places in helper where env->crf[*] was still being set.  Here are the ones that I found:
> 
> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index 3f656e5..e624f97 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -2141,7 +2141,8 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                     \
>          }                                                               \
>      }                                                                   \
>                                                                          \
> -    env->crf[BF(opcode)] = 0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0); \
> +    ppc_set_crf(env, BF(opcode),                                        \
> +                0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0));           \
>  }
> 
>  VSX_TDIV(xstdivdp, 1, float64, VsrD(0), -1022, 1023, 52)
> @@ -2195,7 +2196,8 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                     \
>          }                                                               \
>      }                                                                   \
>                                                                          \
> -    env->crf[BF(opcode)] = 0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0); \
> +    ppc_set_crf(env, BF(opcode),                                        \
> +                0x8 | (fg_flag ? 4 : 0) | (fe_flag ? 2 : 0));           \
>  }
> 
>  VSX_TSQRT(xstsqrtdp, 1, float64, VsrD(0), -1022, 52)
> @@ -2358,7 +2360,7 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                      \
>                                                                           \
>      env->fpscr &= ~(0x0F << FPSCR_FPRF);                                 \
>      env->fpscr |= cc << FPSCR_FPRF;                                      \
> -    env->crf[BF(opcode)] = cc;                                           \
> +    ppc_set_crf(env, BF(opcode), cc);                                   \
>                                                                           \
>      helper_float_check_status(env);                                      \
>  }
> @@ -2450,7 +2452,8 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                       \
>                                                                            \
>      putVSR(xT(opcode), &xt, env);                                         \
>      if ((opcode >> (31-21)) & 1) {                                        \
> -        env->crf[6] = (all_true ? 0x8 : 0) | (all_false ? 0x2 : 0);       \
> +        ppc_set_crf(env, 6,                                               \
> +                    (all_true ? 0x8 : 0) | (all_false ? 0x2 : 0));        \
>      }                                                                     \
>      helper_float_check_status(env);                                       \
>   }
> 
> 
> 
> Note that I do not have the capability of testing any of the SPE instructions.
>