Patchwork [03/14] target-mips: fix FPU exceptions

login
register
mail settings
Submitter Aurelien Jarno
Date Oct. 9, 2012, 8:27 p.m.
Message ID <1349814458-21739-4-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/190427/
State New
Headers show

Comments

Aurelien Jarno - Oct. 9, 2012, 8:27 p.m.
For each FPU instruction that can trigger an FPU exception, it is needed
to reset the softfloat status before and call update_fcr31() after.

Remove the manual NaN assignment in case of float to float operation, as
softfloat is already taking care of that. However for float to int
operation, the value has to be changed to the MIPS one. In the cvtpw_ps
case, the two registers have to be handled separately to guarantee
a correct final value in both registers.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-mips/op_helper.c |   34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
Richard Henderson - Oct. 10, 2012, 8:05 p.m.
On 10/09/2012 01:27 PM, Aurelien Jarno wrote:
> -    return float64_sqrt(fdt0, &env->active_fpu.fp_status);
> +    set_float_exception_flags(0, &env->active_fpu.fp_status);
> +    fdt0 = float64_sqrt(fdt0, &env->active_fpu.fp_status);
> +    update_fcr31(env);
> +    return fdt0;

While accurate, I can't help but think there'd be less memory traffic
if the invariant "fp_status == 0" is maintained between insns.  Thus
you don't need to reset the flags to 0 in each insn, merely change

static inline void update_fcr31(CPUMIPSState *env)
{
    int tmp = ieee_ex_to_mips(get_float_exception_flags(&env->active_fpu.fp_status));
    if (tmp) {
        set_float_exception_flags(0, &env->active_fpu.fp_status);
        SET_FP_CAUSE(env->active_fpu.fcr31, tmp);
        if (GET_FP_ENABLE(env->active_fpu.fcr31) & tmp) {
            helper_raise_exception(env, EXCP_FPE);
        } else {
            UPDATE_FP_FLAGS(env->active_fpu.fcr31, tmp);
        }
    }
}

I'll also note that we don't get the proper PC for the trap there.
We don't save the PC in the translator before the insn, or, more
properly invoke do_restore_state.  That clearly ought to be a
separate change, however.


r~

Patch

diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 9d6d54a..bd3c37c 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2444,12 +2444,18 @@  static inline void update_fcr31(CPUMIPSState *env)
 /* unary operations, modifying fp status  */
 uint64_t helper_float_sqrt_d(CPUMIPSState *env, uint64_t fdt0)
 {
-    return float64_sqrt(fdt0, &env->active_fpu.fp_status);
+    set_float_exception_flags(0, &env->active_fpu.fp_status);
+    fdt0 = float64_sqrt(fdt0, &env->active_fpu.fp_status);
+    update_fcr31(env);
+    return fdt0;
 }
 
 uint32_t helper_float_sqrt_s(CPUMIPSState *env, uint32_t fst0)
 {
-    return float32_sqrt(fst0, &env->active_fpu.fp_status);
+    set_float_exception_flags(0, &env->active_fpu.fp_status);
+    fst0 = float32_sqrt(fst0, &env->active_fpu.fp_status);
+    update_fcr31(env);
+    return fst0;
 }
 
 uint64_t helper_float_cvtd_s(CPUMIPSState *env, uint32_t fst0)
@@ -2522,15 +2528,25 @@  uint64_t helper_float_cvtpw_ps(CPUMIPSState *env, uint64_t fdt0)
 {
     uint32_t wt2;
     uint32_t wth2;
+    int excp, excph;
 
     set_float_exception_flags(0, &env->active_fpu.fp_status);
     wt2 = float32_to_int32(fdt0 & 0XFFFFFFFF, &env->active_fpu.fp_status);
-    wth2 = float32_to_int32(fdt0 >> 32, &env->active_fpu.fp_status);
-    update_fcr31(env);
-    if (GET_FP_CAUSE(env->active_fpu.fcr31) & (FP_OVERFLOW | FP_INVALID)) {
+    excp = get_float_exception_flags(&env->active_fpu.fp_status);
+    if (excp & (float_flag_overflow | float_flag_invalid)) {
         wt2 = FLOAT_SNAN32;
+    }
+
+    set_float_exception_flags(0, &env->active_fpu.fp_status);
+    wth2 = float32_to_int32(fdt0 >> 32, &env->active_fpu.fp_status);
+    excph = get_float_exception_flags(&env->active_fpu.fp_status);
+    if (excph & (float_flag_overflow | float_flag_invalid)) {
         wth2 = FLOAT_SNAN32;
     }
+
+    set_float_exception_flags(excp | excph, &env->active_fpu.fp_status);
+    update_fcr31(env);
+
     return ((uint64_t)wth2 << 32) | wt2;
 }
 
@@ -2970,8 +2986,6 @@  uint64_t helper_float_ ## name ## _d(CPUMIPSState *env,            \
     set_float_exception_flags(0, &env->active_fpu.fp_status);            \
     dt2 = float64_ ## name (fdt0, fdt1, &env->active_fpu.fp_status);     \
     update_fcr31(env);                                             \
-    if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID)                \
-        dt2 = FLOAT_QNAN64;                                        \
     return dt2;                                                    \
 }                                                                  \
                                                                    \
@@ -2983,8 +2997,6 @@  uint32_t helper_float_ ## name ## _s(CPUMIPSState *env,            \
     set_float_exception_flags(0, &env->active_fpu.fp_status);            \
     wt2 = float32_ ## name (fst0, fst1, &env->active_fpu.fp_status);     \
     update_fcr31(env);                                             \
-    if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID)                \
-        wt2 = FLOAT_QNAN32;                                        \
     return wt2;                                                    \
 }                                                                  \
                                                                    \
@@ -3003,10 +3015,6 @@  uint64_t helper_float_ ## name ## _ps(CPUMIPSState *env,           \
     wt2 = float32_ ## name (fst0, fst1, &env->active_fpu.fp_status);     \
     wth2 = float32_ ## name (fsth0, fsth1, &env->active_fpu.fp_status);  \
     update_fcr31(env);                                             \
-    if (GET_FP_CAUSE(env->active_fpu.fcr31) & FP_INVALID) {              \
-        wt2 = FLOAT_QNAN32;                                        \
-        wth2 = FLOAT_QNAN32;                                       \
-    }                                                              \
     return ((uint64_t)wth2 << 32) | wt2;                           \
 }