diff mbox

[v2,9/9] target-i386: add support for FPU exceptions

Message ID 1306186971-9528-10-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno May 23, 2011, 9:42 p.m. UTC 
This patch adds support for FPU exceptions. It keeps the exception in
the softfloat status, and copy them back to env->fpus when needed by
oring them. When loading a new value to env->fpus, it starts with a
clean softfloat status.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-i386/cpu.h       |   15 +++++++++++++++
 target-i386/exec.h      |   12 ------------
 target-i386/helper.c    |   36 ++++++++++++++++++++++++++++++++++++
 target-i386/machine.c   |    3 ++-
 target-i386/op_helper.c |   32 ++++++++++++++++----------------
 5 files changed, 69 insertions(+), 29 deletions(-)

Comments

Peter Maydell May 24, 2011, 3:55 p.m. UTC | #1 
On 23 May 2011 22:42, Aurelien Jarno <aurelien@aurel32.net> wrote:
> This patch adds support for FPU exceptions. It keeps the exception in
> the softfloat status, and copy them back to env->fpus when needed by
> oring them. When loading a new value to env->fpus, it starts with a
> clean softfloat status.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

gdbstub.c still looks at env->fpus directly without calling
cpu_x86_update_fpus() first; it should probably go through
helper_fnstsw() now (or if you like a utility function that does what
helper_fnstsw() does now).

Similarly the code in helper_fstenv() and cpu_pre_save() that does:
    cpu_x86_update_fpus(env);
    SOMETHING = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;

could be usefully abstracted away into a call to this utility fn.

For the rest, it looks OK, but the cpu_x86_update_fpus() and
cpu_x86_set_fpus() functions feel a bit like they're only half
an abstraction layer -- so sometimes it's OK to tweak env->fpus
directly and sometimes you need to use the functions. That means
it's tricky to tell from eyeballing or grepping the code whether an
update_fpus() call got missed out. Maybe it would be better to have
a set of functions such that you could mandate that all env->fpus
accesses went via the functions?

A random nitpick:

> @@ -4208,7 +4200,13 @@ void helper_fscale(void)
>     if (floatx80_is_any_nan(ST1)) {
>         ST0 = ST1;
>     } else {
> -        int n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
> +        int n, x;
> +
> +        /* The float to int conversion should not generate any exception. */
> +        x = get_float_exception_flags(&env->fp_status);
> +        n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
> +        set_float_exception_flags(x, &env->fp_status);
> +
>         ST0 = floatx80_scalbn(ST0, n, &env->fp_status);
>     }
>  }

...doesn't this mean you won't set the #D flag if ST1 is denormal?

I'm unconvinced about scalbn as a softfloat primitive. It gets used:
 * by ARM for the VCVT fixed-point conversions, as a combination of
   scalbn + float-to-int or int-to-float + scalbn
 * by PPC for the same reason (vctuxs, vctsxs, vcfux, vcfsx)
 * by x86 here as a float-to-int + scalbn, for FSCALE

and I think that in all three cases the attempt to implement a floating
point op by composing two softfloat primitives causes problems with
getting the floating point exception flags right. (In the fixedpoint
conversion case the issue is that for instance if the input is a
negative number we should only set InvalidOp but the scalbn is likely
to result in our also setting Inexact. But if you don't let the
scalbn set any flags at all then you have to special case when the
input is a NaN or denormal, which is equally awkward.)

-- PMM
diff mbox

Patch

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fe65886..d216356 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -121,6 +121,19 @@ 
 #define VIP_MASK                0x00100000
 #define ID_MASK                 0x00200000
 
+/* FPU flags */
+#define FPUS_IE (1 << 0)
+#define FPUS_DE (1 << 1)
+#define FPUS_ZE (1 << 2)
+#define FPUS_OE (1 << 3)
+#define FPUS_UE (1 << 4)
+#define FPUS_PE (1 << 5)
+#define FPUS_SF (1 << 6)
+#define FPUS_SE (1 << 7)
+#define FPUS_B  (1 << 15)
+
+#define FPUC_EM 0x3f
+
 /* hidden flags - used internally by qemu to represent additional cpu
    states. Only the CPL, INHIBIT_IRQ, SMM and SVMI are not
    redundant. We avoid using the IOPL_MASK, TF_MASK and VM_MASK bit
@@ -877,6 +890,8 @@  void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
 
 /* helper.c */
+void cpu_x86_set_fpus(CPUX86State *s, uint16_t val);
+void cpu_x86_update_fpus(CPUX86State *s);
 int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
                              int is_write, int mmu_idx, int is_softmmu);
 #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
diff --git a/target-i386/exec.h b/target-i386/exec.h
index 9bd080e..e14d0d9 100644
--- a/target-i386/exec.h
+++ b/target-i386/exec.h
@@ -144,18 +144,6 @@  static inline void helper_fstt(floatx80 f, target_ulong ptr)
     stw(ptr + 8, temp.l.upper);
 }
 
-#define FPUS_IE (1 << 0)
-#define FPUS_DE (1 << 1)
-#define FPUS_ZE (1 << 2)
-#define FPUS_OE (1 << 3)
-#define FPUS_UE (1 << 4)
-#define FPUS_PE (1 << 5)
-#define FPUS_SF (1 << 6)
-#define FPUS_SE (1 << 7)
-#define FPUS_B  (1 << 15)
-
-#define FPUC_EM 0x3f
-
 static inline uint32_t compute_eflags(void)
 {
     return env->eflags | helper_cc_compute_all(CC_OP) | (DF & DF_MASK);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 5c4b288..23646d3 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -396,6 +396,7 @@  void cpu_dump_state(CPUState *env, FILE *f, fprintf_function cpu_fprintf,
         for(i = 0; i < 8; i++) {
             fptag |= ((!env->fptags[i]) << i);
         }
+        cpu_x86_update_fpus(env);
         cpu_fprintf(f, "FCW=%04x FSW=%04x [ST=%d] FTW=%02x MXCSR=%08x\n",
                     env->fpuc,
                     (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11,
@@ -1238,6 +1239,41 @@  int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
     return 1;
 }
 
+void cpu_x86_set_fpus(CPUX86State *s, uint16_t val)
+{
+    set_float_exception_flags(0, &s->fp_status);
+    s->fpus = val;
+}
+
+void cpu_x86_update_fpus(CPUX86State *s)
+{
+    int xcpt = get_float_exception_flags(&s->fp_status);
+
+    if (xcpt) {
+        if (xcpt & float_flag_invalid) {
+            s->fpus |= FPUS_IE;
+        }
+        if (xcpt & float_flag_input_denormal) {
+            s->fpus |= FPUS_DE;
+        }
+        if (xcpt & float_flag_divbyzero) {
+            s->fpus |= FPUS_ZE;
+        }
+        if (xcpt & float_flag_overflow) {
+            s->fpus |= FPUS_OE;
+        }
+        if (xcpt & float_flag_underflow) {
+            s->fpus |= FPUS_UE;
+        }
+        if (xcpt & float_flag_inexact) {
+            s->fpus |= FPUS_PE;
+        }
+        if (s->fpus & (~s->fpuc & FPUC_EM)) {
+            s->fpus |= FPUS_SE | FPUS_B;
+        }
+    }
+}
+
 CPUX86State *cpu_x86_init(const char *cpu_model)
 {
     CPUX86State *env;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index bbeae88..82c5c01 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -241,6 +241,7 @@  static void cpu_pre_save(void *opaque)
     int i;
 
     /* FPU */
+    cpu_x86_update_fpus(env);
     env->fpus_vmstate = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
     env->fptag_vmstate = 0;
     for(i = 0; i < 8; i++) {
@@ -257,7 +258,7 @@  static int cpu_post_load(void *opaque, int version_id)
 
     /* XXX: restore FPU round state */
     env->fpstt = (env->fpus_vmstate >> 11) & 7;
-    env->fpus = env->fpus_vmstate & ~0x3800;
+    cpu_x86_set_fpus(env, env->fpus_vmstate & ~0x3800);
     env->fptag_vmstate ^= 0xff;
     for(i = 0; i < 8; i++) {
         env->fptags[i] = (env->fptag_vmstate >> i) & 1;
diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index 8ba2b5f..eccb957 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -3447,18 +3447,8 @@  static inline floatx80 double_to_floatx80(double a)
     return float64_to_floatx80(u.f64, &env->fp_status);
 }
 
-static void fpu_set_exception(int mask)
-{
-    env->fpus |= mask;
-    if (env->fpus & (~env->fpuc & FPUC_EM))
-        env->fpus |= FPUS_SE | FPUS_B;
-}
-
 static inline floatx80 helper_fdiv(floatx80 a, floatx80 b)
 {
-    if (floatx80_is_zero(b)) {
-        fpu_set_exception(FPUS_ZE);
-    }
     return floatx80_div(a, b, &env->fp_status);
 }
 
@@ -3845,6 +3835,7 @@  void helper_fldz_FT0(void)
 
 uint32_t helper_fnstsw(void)
 {
+    cpu_x86_update_fpus(env);
     return (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
 }
 
@@ -3897,18 +3888,19 @@  void helper_fldcw(uint32_t val)
 
 void helper_fclex(void)
 {
-    env->fpus &= 0x7f00;
+    cpu_x86_set_fpus(env, env->fpus & 0x7f00);
 }
 
 void helper_fwait(void)
 {
+    cpu_x86_update_fpus(env);
     if (env->fpus & FPUS_SE)
         fpu_raise_exception();
 }
 
 void helper_fninit(void)
 {
-    env->fpus = 0;
+    cpu_x86_set_fpus(env, 0);
     env->fpstt = 0;
     env->fpuc = 0x37f;
     env->fptags[0] = 1;
@@ -4208,7 +4200,13 @@  void helper_fscale(void)
     if (floatx80_is_any_nan(ST1)) {
         ST0 = ST1;
     } else {
-        int n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
+        int n, x;
+
+        /* The float to int conversion should not generate any exception. */
+        x = get_float_exception_flags(&env->fp_status);
+        n = floatx80_to_int32_round_to_zero(ST1, &env->fp_status);
+        set_float_exception_flags(x, &env->fp_status);
+
         ST0 = floatx80_scalbn(ST0, n, &env->fp_status);
     }
 }
@@ -4267,6 +4265,7 @@  void helper_fstenv(target_ulong ptr, int data32)
     uint64_t mant;
     CPU_LDoubleU tmp;
 
+    cpu_x86_update_fpus(env);
     fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
     fptag = 0;
     for (i=7; i>=0; i--) {
@@ -4324,7 +4323,7 @@  void helper_fldenv(target_ulong ptr, int data32)
         fptag = lduw(ptr + 4);
     }
     env->fpstt = (fpus >> 11) & 7;
-    env->fpus = fpus & ~0x3800;
+    cpu_x86_set_fpus(env, fpus & ~0x3800);
     for(i = 0;i < 8; i++) {
         env->fptags[i] = ((fptag & 3) == 3);
         fptag >>= 2;
@@ -4346,7 +4345,7 @@  void helper_fsave(target_ulong ptr, int data32)
     }
 
     /* fninit */
-    env->fpus = 0;
+    cpu_x86_set_fpus(env, 0);
     env->fpstt = 0;
     env->fpuc = 0x37f;
     env->fptags[0] = 1;
@@ -4385,6 +4384,7 @@  void helper_fxsave(target_ulong ptr, int data64)
         raise_exception(EXCP0D_GPF);
     }
 
+    cpu_x86_update_fpus(env);
     fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
     fptag = 0;
     for(i = 0; i < 8; i++) {
@@ -4450,7 +4450,7 @@  void helper_fxrstor(target_ulong ptr, int data64)
     fpus = lduw(ptr + 2);
     fptag = lduw(ptr + 4);
     env->fpstt = (fpus >> 11) & 7;
-    env->fpus = fpus & ~0x3800;
+    cpu_x86_set_fpus(env, fpus & ~0x3800);
     fptag ^= 0xff;
     for(i = 0;i < 8; i++) {
         env->fptags[i] = ((fptag >> i) & 1);