[Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Richard Henderson]

Beginning with John Arbuckle's fdiv test case, clean up some
of the fp helpers.  As with fdiv, fre and fresqrt are missing
divide-by-zero exceptions.

I've also noticed that load/store were using arithmetic conversions
to/from float32.  These should be using the non-arithmetic algorithms
listed in the manual.

Aside from cleaning up all of the rest of the helpers, I believe that
the implementation of the single-precision operations are incorrect.
They are currently implemented with the double-precision operation
followed by a round-to-single.  This causes incorrect results via
double rounding.  I believe better results could be had by using
these non-arithmetic converters to produce float32 operands, use the
proper float32 softfloat operations, and then convert back.

Anyway, all of this has been broken long enough that it'll need to
wait til next devel cycle before anything further gets done.


r~


Richard Henderson (7):
  target/ppc: Enable fp exceptions for user-only
  target/ppc: Honor fpscr_ze semantics and tidy fdiv
  target/ppc: Tidy helper_fmul
  target/ppc: Tidy helper_fadd, helper_fsub
  target/ppc: Tidy helper_fsqrt
  target/ppc: Honor fpscr_ze semantics and tidy fre, fresqrt
  target/ppc: Use non-arithmetic conversions for fp load/store

 target/ppc/helper.h                |  14 +-
 target/ppc/fpu_helper.c            | 294 +++++++++++++++++------------
 target/ppc/translate/fp-impl.inc.c |  26 +--
 target/ppc/translate_init.inc.c    |   2 +
 4 files changed, 189 insertions(+), 147 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/7] target/ppc: Enable fp exceptions for user-only

[Richard Henderson]

While just setting the MSR bits is sufficient, we can tidy
the helper code by extracting the MSR test to a helper and
then forcing it true for user-only.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/fpu_helper.c         | 15 ++++++++++++---
 target/ppc/translate_init.inc.c |  2 ++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 7714bfe0f9..119826b5a7 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -36,6 +36,15 @@ static inline float128 float128_snan_to_qnan(float128 x)
 #define float32_snan_to_qnan(x) ((x) | 0x00400000)
 #define float16_snan_to_qnan(x) ((x) | 0x0200)
 
+static inline bool fp_exceptions_enabled(CPUPPCState *env)
+{
+#ifdef CONFIG_USER_ONLY
+    return true;
+#else
+    return (env->msr & ((1U << MSR_FE0) | (1U << MSR_FE1))) != 0;
+#endif
+}
+
 /*****************************************************************************/
 /* Floating point operations helpers */
 uint64_t helper_float32_to_float64(CPUPPCState *env, uint32_t arg)
@@ -207,7 +216,7 @@ uint64_t float_invalid_op_excp(CPUPPCState *env, int op, int set_fpcc)
     if (ve != 0) {
         /* Update the floating-point enabled exception summary */
         env->fpscr |= 1 << FPSCR_FEX;
-        if (msr_fe0 != 0 || msr_fe1 != 0) {
+        if (fp_exceptions_enabled(env)) {
             /* GETPC() works here because this is inline */
             raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
                                    POWERPC_EXCP_FP | op, GETPC());
@@ -225,7 +234,7 @@ static inline void float_zero_divide_excp(CPUPPCState *env, uintptr_t raddr)
     if (fpscr_ze != 0) {
         /* Update the floating-point enabled exception summary */
         env->fpscr |= 1 << FPSCR_FEX;
-        if (msr_fe0 != 0 || msr_fe1 != 0) {
+        if (fp_exceptions_enabled(env)) {
             raise_exception_err_ra(env, POWERPC_EXCP_PROGRAM,
                                    POWERPC_EXCP_FP | POWERPC_EXCP_FP_ZX,
                                    raddr);
@@ -547,7 +556,7 @@ static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
     if (cs->exception_index == POWERPC_EXCP_PROGRAM &&
         (env->error_code & POWERPC_EXCP_FP)) {
         /* Differred floating-point exception after target FPR update */
-        if (msr_fe0 != 0 || msr_fe1 != 0) {
+        if (fp_exceptions_enabled(env)) {
             raise_exception_err_ra(env, cs->exception_index,
                                    env->error_code, raddr);
         }
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 76d6f3fd5e..ffa74a1026 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10278,6 +10278,8 @@ static void ppc_cpu_reset(CPUState *s)
 #endif
 #if defined(CONFIG_USER_ONLY)
     msr |= (target_ulong)1 << MSR_FP; /* Allow floating point usage */
+    msr |= (target_ulong)1 << MSR_FE0; /* Allow floating point exceptions */
+    msr |= (target_ulong)1 << MSR_FE1;
     msr |= (target_ulong)1 << MSR_VR; /* Allow altivec usage */
     msr |= (target_ulong)1 << MSR_VSX; /* Allow VSX usage */
     msr |= (target_ulong)1 << MSR_SPE; /* Allow SPE usage */
-- 
2.17.1

[Qemu-devel] [PATCH 2/7] target/ppc: Honor fpscr_ze semantics and tidy fdiv

[Richard Henderson]

Divide by zero, exception taken, leaves the destination register
unmodified.  Therefore we must raise the exception before returning
from helper_fdiv.  Move the check from do_float_check_status into
helper_fdiv.

At the same time, tidy the invalid exception checking so that we
rely on softfloat for initial argument validation, and select the
kind of invalid operand exception only when we know we must.

At the same time, pass and return float64 values directly rather
than bounce through the CPU_DoubleU union.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper.h     |  2 +-
 target/ppc/fpu_helper.c | 44 ++++++++++++++++++++---------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index d751f0e219..151437d8fc 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -88,7 +88,7 @@ DEF_HELPER_2(frim, i64, env, i64)
 DEF_HELPER_3(fadd, i64, env, i64, i64)
 DEF_HELPER_3(fsub, i64, env, i64, i64)
 DEF_HELPER_3(fmul, i64, env, i64, i64)
-DEF_HELPER_3(fdiv, i64, env, i64, i64)
+DEF_HELPER_3(fdiv, f64, env, f64, f64)
 DEF_HELPER_4(fmadd, i64, env, i64, i64, i64)
 DEF_HELPER_4(fmsub, i64, env, i64, i64, i64)
 DEF_HELPER_4(fnmadd, i64, env, i64, i64, i64)
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 119826b5a7..0df7e31c10 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -543,9 +543,7 @@ static void do_float_check_status(CPUPPCState *env, uintptr_t raddr)
     CPUState *cs = CPU(ppc_env_get_cpu(env));
     int status = get_float_exception_flags(&env->fp_status);
 
-    if (status & float_flag_divbyzero) {
-        float_zero_divide_excp(env, raddr);
-    } else if (status & float_flag_overflow) {
+    if (status & float_flag_overflow) {
         float_overflow_excp(env);
     } else if (status & float_flag_underflow) {
         float_underflow_excp(env);
@@ -653,30 +651,32 @@ uint64_t helper_fmul(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
 }
 
 /* fdiv - fdiv. */
-uint64_t helper_fdiv(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
+float64 helper_fdiv(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    CPU_DoubleU farg1, farg2;
+    float64 ret = float64_div(arg1, arg2, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-
-    if (unlikely(float64_is_infinity(farg1.d) &&
-                 float64_is_infinity(farg2.d))) {
-        /* Division of infinity by infinity */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIDI, 1);
-    } else if (unlikely(float64_is_zero(farg1.d) && float64_is_zero(farg2.d))) {
-        /* Division of zero by zero */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXZDZ, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status))) {
-            /* sNaN division */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+    if (unlikely(status)) {
+        if (status & float_flag_invalid) {
+            /* Determine what kind of invalid operation was seen.  */
+            if (float64_is_infinity(arg1) && float64_is_infinity(arg2)) {
+                /* Division of infinity by infinity */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIDI, 1);
+            } else if (float64_is_zero(arg1) && float64_is_zero(arg2)) {
+                /* Division of zero by zero */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXZDZ, 1);
+            } else if (float64_is_signaling_nan(arg1, &env->fp_status) ||
+                       float64_is_signaling_nan(arg2, &env->fp_status)) {
+                /* sNaN division */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            }
+        }
+        if (status & float_flag_divbyzero) {
+            float_zero_divide_excp(env, GETPC());
         }
-        farg1.d = float64_div(farg1.d, farg2.d, &env->fp_status);
     }
 
-    return farg1.ll;
+    return ret;
 }
 
 
-- 
2.17.1

[Qemu-devel] [PATCH 3/7] target/ppc: Tidy helper_fmul

[Richard Henderson]

Tidy the invalid exception checking so that we rely on softfloat for
initial argument validation, and select the kind of invalid operand
exception only when we know we must.  Pass and return float64 values
directly rather than bounce through the CPU_DoubleU union.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper.h     |  2 +-
 target/ppc/fpu_helper.c | 25 +++++++++++--------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 151437d8fc..7ddbc0fc19 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -87,7 +87,7 @@ DEF_HELPER_2(frim, i64, env, i64)
 
 DEF_HELPER_3(fadd, i64, env, i64, i64)
 DEF_HELPER_3(fsub, i64, env, i64, i64)
-DEF_HELPER_3(fmul, i64, env, i64, i64)
+DEF_HELPER_3(fmul, f64, env, f64, f64)
 DEF_HELPER_3(fdiv, f64, env, f64, f64)
 DEF_HELPER_4(fmadd, i64, env, i64, i64, i64)
 DEF_HELPER_4(fmsub, i64, env, i64, i64, i64)
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 0df7e31c10..5642734b5b 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -627,27 +627,24 @@ uint64_t helper_fsub(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
 }
 
 /* fmul - fmul. */
-uint64_t helper_fmul(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
+float64 helper_fmul(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    CPU_DoubleU farg1, farg2;
+    float64 ret = float64_mul(arg1, arg2, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-
-    if (unlikely((float64_is_infinity(farg1.d) && float64_is_zero(farg2.d)) ||
-                 (float64_is_zero(farg1.d) && float64_is_infinity(farg2.d)))) {
-        /* Multiplication of zero by infinity */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status))) {
+    if (unlikely(status & float_flag_invalid)) {
+        if ((float64_is_infinity(arg1) && float64_is_zero(arg2)) ||
+            (float64_is_zero(arg1) && float64_is_infinity(arg2))) {
+            /* Multiplication of zero by infinity */
+            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXIMZ, 1);
+        } else if (float64_is_signaling_nan(arg1, &env->fp_status) ||
+                   float64_is_signaling_nan(arg2, &env->fp_status)) {
             /* sNaN multiplication */
             float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
         }
-        farg1.d = float64_mul(farg1.d, farg2.d, &env->fp_status);
     }
 
-    return farg1.ll;
+    return ret;
 }
 
 /* fdiv - fdiv. */
-- 
2.17.1

[Qemu-devel] [PATCH 4/7] target/ppc: Tidy helper_fadd, helper_fsub

[Richard Henderson]

Tidy the invalid exception checking so that we rely on softfloat for
initial argument validation, and select the kind of invalid operand
exception only when we know we must.  Pass and return float64 values
directly rather than bounce through the CPU_DoubleU union.

Note that because we know float_flag_invalid was set, we do not have
to re-check the signs of the infinities.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper.h     |  4 ++--
 target/ppc/fpu_helper.c | 50 +++++++++++++++++------------------------
 2 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 7ddbc0fc19..3262e2feaf 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -85,8 +85,8 @@ DEF_HELPER_2(friz, i64, env, i64)
 DEF_HELPER_2(frip, i64, env, i64)
 DEF_HELPER_2(frim, i64, env, i64)
 
-DEF_HELPER_3(fadd, i64, env, i64, i64)
-DEF_HELPER_3(fsub, i64, env, i64, i64)
+DEF_HELPER_3(fadd, f64, env, f64, f64)
+DEF_HELPER_3(fsub, f64, env, f64, f64)
 DEF_HELPER_3(fmul, f64, env, f64, f64)
 DEF_HELPER_3(fdiv, f64, env, f64, f64)
 DEF_HELPER_4(fmadd, i64, env, i64, i64, i64)
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 5642734b5b..2d56c93498 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -579,51 +579,43 @@ void helper_reset_fpstatus(CPUPPCState *env)
 }
 
 /* fadd - fadd. */
-uint64_t helper_fadd(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
+float64 helper_fadd(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    CPU_DoubleU farg1, farg2;
+    float64 ret = float64_add(arg1, arg2, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-
-    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
-                 float64_is_neg(farg1.d) != float64_is_neg(farg2.d))) {
-        /* Magnitude subtraction of infinities */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status))) {
+    if (unlikely(status & float_flag_invalid)) {
+        if (float64_is_infinity(arg1) && float64_is_infinity(arg2)) {
+            /* Magnitude subtraction of infinities */
+            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
+        } else if (float64_is_signaling_nan(arg1, &env->fp_status) ||
+                   float64_is_signaling_nan(arg2, &env->fp_status)) {
             /* sNaN addition */
             float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
         }
-        farg1.d = float64_add(farg1.d, farg2.d, &env->fp_status);
     }
 
-    return farg1.ll;
+    return ret;
 }
 
 /* fsub - fsub. */
-uint64_t helper_fsub(CPUPPCState *env, uint64_t arg1, uint64_t arg2)
+float64 helper_fsub(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    CPU_DoubleU farg1, farg2;
+    float64 ret = float64_sub(arg1, arg2, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg1.ll = arg1;
-    farg2.ll = arg2;
-
-    if (unlikely(float64_is_infinity(farg1.d) && float64_is_infinity(farg2.d) &&
-                 float64_is_neg(farg1.d) == float64_is_neg(farg2.d))) {
-        /* Magnitude subtraction of infinities */
-        farg1.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
-    } else {
-        if (unlikely(float64_is_signaling_nan(farg1.d, &env->fp_status) ||
-                     float64_is_signaling_nan(farg2.d, &env->fp_status))) {
-            /* sNaN subtraction */
+    if (unlikely(status & float_flag_invalid)) {
+        if (float64_is_infinity(arg1) && float64_is_infinity(arg2)) {
+            /* Magnitude subtraction of infinities */
+            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXISI, 1);
+        } else if (float64_is_signaling_nan(arg1, &env->fp_status) ||
+                   float64_is_signaling_nan(arg2, &env->fp_status)) {
+            /* sNaN addition */
             float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
         }
-        farg1.d = float64_sub(farg1.d, farg2.d, &env->fp_status);
     }
 
-    return farg1.ll;
+    return ret;
 }
 
 /* fmul - fmul. */
-- 
2.17.1

[Qemu-devel] [PATCH 5/7] target/ppc: Tidy helper_fsqrt

[Richard Henderson]

Tidy the invalid exception checking so that we rely on softfloat for
initial argument validation, and select the kind of invalid operand
exception only when we know we must.  Pass and return float64 values
directly rather than bounce through the CPU_DoubleU union.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper.h     |  2 +-
 target/ppc/fpu_helper.c | 29 ++++++++++++++---------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 3262e2feaf..cc3d031407 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -93,7 +93,7 @@ DEF_HELPER_4(fmadd, i64, env, i64, i64, i64)
 DEF_HELPER_4(fmsub, i64, env, i64, i64, i64)
 DEF_HELPER_4(fnmadd, i64, env, i64, i64, i64)
 DEF_HELPER_4(fnmsub, i64, env, i64, i64, i64)
-DEF_HELPER_2(fsqrt, i64, env, i64)
+DEF_HELPER_2(fsqrt, f64, env, f64)
 DEF_HELPER_2(fre, i64, env, i64)
 DEF_HELPER_2(fres, i64, env, i64)
 DEF_HELPER_2(frsqrte, i64, env, i64)
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 2d56c93498..c8a2dd6408 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -850,25 +850,24 @@ uint64_t helper_frsp(CPUPPCState *env, uint64_t arg)
 }
 
 /* fsqrt - fsqrt. */
-uint64_t helper_fsqrt(CPUPPCState *env, uint64_t arg)
+float64 helper_fsqrt(CPUPPCState *env, float64 arg)
 {
-    CPU_DoubleU farg;
+    float64 ret = float64_sqrt(arg, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg.ll = arg;
-
-    if (unlikely(float64_is_any_nan(farg.d))) {
-        if (unlikely(float64_is_signaling_nan(farg.d, &env->fp_status))) {
-            /* sNaN reciprocal square root */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
-            farg.ll = float64_snan_to_qnan(farg.ll);
+    if (unlikely(status & float_flag_invalid)) {
+        if (unlikely(float64_is_any_nan(arg))) {
+            if (unlikely(float64_is_signaling_nan(arg, &env->fp_status))) {
+                /* sNaN square root */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            }
+        } else {
+            /* Square root of a negative nonzero number */
+            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSQRT, 1);
         }
-    } else if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
-        /* Square root of a negative nonzero number */
-        farg.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSQRT, 1);
-    } else {
-        farg.d = float64_sqrt(farg.d, &env->fp_status);
     }
-    return farg.ll;
+
+    return ret;
 }
 
 /* fre - fre. */
-- 
2.17.1

[Qemu-devel] [PATCH 6/7] target/ppc: Honor fpscr_ze semantics and tidy fre, fresqrt

[Richard Henderson]

Divide by zero, exception taken, leaves the destination register
unmodified.  Therefore we must raise the exception before returning
from the respective helpers.

>From helper_fre, divide by zero exception not taken, return the
documented +/- 0.5.

At the same time, tidy the invalid exception checking so that we
rely on softfloat for initial argument validation, and select the
kind of invalid operand exception only when we know we must.

At the same time, pass and return float64 values directly rather
than bounce through the CPU_DoubleU union.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/fpu_helper.c | 62 ++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index c8a2dd6408..1e195487d3 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -871,18 +871,27 @@ float64 helper_fsqrt(CPUPPCState *env, float64 arg)
 }
 
 /* fre - fre. */
-uint64_t helper_fre(CPUPPCState *env, uint64_t arg)
+float64 helper_fre(CPUPPCState *env, float64 arg)
 {
-    CPU_DoubleU farg;
+    /* "Estimate" the reciprocal with actual division.  */
+    float64 ret = float64_div(float64_one, arg, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg.ll = arg;
-
-    if (unlikely(float64_is_signaling_nan(farg.d, &env->fp_status))) {
-        /* sNaN reciprocal */
-        float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+    if (unlikely(status)) {
+        if (status & float_flag_invalid) {
+            if (float64_is_signaling_nan(arg, &env->fp_status)) {
+                /* sNaN reciprocal */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            }
+        }
+        if (status & float_flag_divbyzero) {
+            float_zero_divide_excp(env, GETPC());
+            /* For FPSCR.ZE == 0, the result is 1/2.  */
+            ret = float64_set_sign(float64_half, float64_is_neg(arg));
+        }
     }
-    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
-    return farg.d;
+
+    return ret;
 }
 
 /* fres - fres. */
@@ -905,27 +914,30 @@ uint64_t helper_fres(CPUPPCState *env, uint64_t arg)
 }
 
 /* frsqrte  - frsqrte. */
-uint64_t helper_frsqrte(CPUPPCState *env, uint64_t arg)
+float64 helper_frsqrte(CPUPPCState *env, float64 arg)
 {
-    CPU_DoubleU farg;
+    /* "Estimate" the reciprocal with actual division.  */
+    float64 rets = float64_sqrt(arg, &env->fp_status);
+    float64 retd = float64_div(float64_one, rets, &env->fp_status);
+    int status = get_float_exception_flags(&env->fp_status);
 
-    farg.ll = arg;
-
-    if (unlikely(float64_is_any_nan(farg.d))) {
-        if (unlikely(float64_is_signaling_nan(farg.d, &env->fp_status))) {
-            /* sNaN reciprocal square root */
-            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
-            farg.ll = float64_snan_to_qnan(farg.ll);
+    if (unlikely(status)) {
+        if (status & float_flag_invalid) {
+            if (float64_is_signaling_nan(arg, &env->fp_status)) {
+                /* sNaN reciprocal */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 1);
+            } else {
+                /* Square root of a negative nonzero number */
+                float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSQRT, 1);
+            }
+        }
+        if (status & float_flag_divbyzero) {
+            /* Reciprocal of (square root of) zero.  */
+            float_zero_divide_excp(env, GETPC());
         }
-    } else if (unlikely(float64_is_neg(farg.d) && !float64_is_zero(farg.d))) {
-        /* Reciprocal square root of a negative nonzero number */
-        farg.ll = float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSQRT, 1);
-    } else {
-        farg.d = float64_sqrt(farg.d, &env->fp_status);
-        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
     }
 
-    return farg.ll;
+    return retd;
 }
 
 /* fsel - fsel. */
-- 
2.17.1

[Qemu-devel] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Richard Henderson]

Memory operations have no side effects on fp state.
The use of a "real" conversions between float64 and float32
would raise exceptions for SNaN and out-of-range inputs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper.h                |  4 +-
 target/ppc/fpu_helper.c            | 63 ++++++++++++++++++++++++------
 target/ppc/translate/fp-impl.inc.c | 26 +++++-------
 3 files changed, 62 insertions(+), 31 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index cc3d031407..33e6e1df60 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -61,8 +61,8 @@ DEF_HELPER_2(compute_fprf_float64, void, env, i64)
 DEF_HELPER_3(store_fpscr, void, env, i64, i32)
 DEF_HELPER_2(fpscr_clrbit, void, env, i32)
 DEF_HELPER_2(fpscr_setbit, void, env, i32)
-DEF_HELPER_2(float64_to_float32, i32, env, i64)
-DEF_HELPER_2(float32_to_float64, i64, env, i32)
+DEF_HELPER_FLAGS_1(todouble, TCG_CALL_NO_RWG_SE, i64, i32)
+DEF_HELPER_FLAGS_1(tosingle, TCG_CALL_NO_RWG_SE, i32, i64)
 
 DEF_HELPER_4(fcmpo, void, env, i64, i64, i32)
 DEF_HELPER_4(fcmpu, void, env, i64, i64, i32)
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 1e195487d3..d4e9e3bccb 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -47,24 +47,61 @@ static inline bool fp_exceptions_enabled(CPUPPCState *env)
 
 /*****************************************************************************/
 /* Floating point operations helpers */
-uint64_t helper_float32_to_float64(CPUPPCState *env, uint32_t arg)
-{
-    CPU_FloatU f;
-    CPU_DoubleU d;
 
-    f.l = arg;
-    d.d = float32_to_float64(f.f, &env->fp_status);
-    return d.ll;
+/*
+ * This is the non-arithmatic conversion that happens e.g. on loads.
+ * In the Power ISA pseudocode, this is called DOUBLE.
+ */
+uint64_t helper_todouble(uint32_t arg)
+{
+    uint32_t abs_arg = arg & 0x7fffffff;
+    uint64_t ret;
+
+    if (likely(abs_arg >= 0x00800000)) {
+        /* Normalized operand, or Inf, or NaN.  */
+        ret  = (uint64_t)extract32(arg, 30, 2) << 62;
+        ret |= ((extract32(arg, 30, 1) ^ 1) * (uint64_t)7) << 59;
+        ret |= (uint64_t)extract32(arg, 0, 29) << 29;
+    } else {
+        /* Zero or Denormalized operand.  */
+        ret = (uint64_t)extract32(arg, 31, 1) << 63;
+        if (unlikely(abs_arg != 0)) {
+            /* Denormalized operand.  */
+            int shift = clz32(abs_arg) - 9;
+            int exp = -126 - shift + 1023;
+            ret |= (uint64_t)exp << 52;
+            ret |= abs_arg << (shift + 29);
+        }
+    }
+    return ret;
 }
 
-uint32_t helper_float64_to_float32(CPUPPCState *env, uint64_t arg)
+/*
+ * This is the non-arithmatic conversion that happens e.g. on stores.
+ * In the Power ISA pseudocode, this is called SINGLE.
+ */
+uint32_t helper_tosingle(uint64_t arg)
 {
-    CPU_FloatU f;
-    CPU_DoubleU d;
+    int exp = extract64(arg, 52, 11);
+    uint32_t ret;
 
-    d.ll = arg;
-    f.f = float64_to_float32(d.d, &env->fp_status);
-    return f.l;
+    if (likely(exp > 896)) {
+        /* No denormalization required (includes Inf, NaN).  */
+        ret  = extract64(arg, 62, 2) << 30;
+        ret |= extract64(arg, 29, 29);
+    } else {
+        /* Zero or Denormal result.  If the exponent is in bounds for
+         * a single-precision denormal result, extract the proper bits.
+         * If the input is not zero, and the exponent is out of bounds,
+         * then the result is undefined; this underflows to zero.
+         */
+        ret = extract64(arg, 63, 1) << 63;
+        if (unlikely(exp >= 874)) {
+            /* Denormal result.  */
+            ret |= ((1ULL << 52) | extract64(arg, 0, 52)) >> (896 + 30 - exp);
+        }
+    }
+    return ret;
 }
 
 static inline int ppc_float32_get_unbiased_exp(float32 f)
diff --git a/target/ppc/translate/fp-impl.inc.c b/target/ppc/translate/fp-impl.inc.c
index 2fbd4d4f38..a6f522b85c 100644
--- a/target/ppc/translate/fp-impl.inc.c
+++ b/target/ppc/translate/fp-impl.inc.c
@@ -660,15 +660,12 @@ GEN_LDUF(name, ldop, op | 0x21, type);                                        \
 GEN_LDUXF(name, ldop, op | 0x01, type);                                       \
 GEN_LDXF(name, ldop, 0x17, op | 0x00, type)
 
-static inline void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 arg1, TCGv arg2)
+static void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 dest, TCGv addr)
 {
-    TCGv t0 = tcg_temp_new();
-    TCGv_i32 t1 = tcg_temp_new_i32();
-    gen_qemu_ld32u(ctx, t0, arg2);
-    tcg_gen_trunc_tl_i32(t1, t0);
-    tcg_temp_free(t0);
-    gen_helper_float32_to_float64(arg1, cpu_env, t1);
-    tcg_temp_free_i32(t1);
+    TCGv_i32 tmp = tcg_temp_new_i32();
+    tcg_gen_qemu_ld_i32(tmp, addr, ctx->mem_idx, DEF_MEMOP(MO_UL));
+    gen_helper_todouble(dest, tmp);
+    tcg_temp_free_i32(tmp);
 }
 
  /* lfd lfdu lfdux lfdx */
@@ -836,15 +833,12 @@ GEN_STUF(name, stop, op | 0x21, type);                                        \
 GEN_STUXF(name, stop, op | 0x01, type);                                       \
 GEN_STXF(name, stop, 0x17, op | 0x00, type)
 
-static inline void gen_qemu_st32fs(DisasContext *ctx, TCGv_i64 arg1, TCGv arg2)
+static void gen_qemu_st32fs(DisasContext *ctx, TCGv_i64 src, TCGv addr)
 {
-    TCGv_i32 t0 = tcg_temp_new_i32();
-    TCGv t1 = tcg_temp_new();
-    gen_helper_float64_to_float32(t0, cpu_env, arg1);
-    tcg_gen_extu_i32_tl(t1, t0);
-    tcg_temp_free_i32(t0);
-    gen_qemu_st32(ctx, t1, arg2);
-    tcg_temp_free(t1);
+    TCGv_i32 tmp = tcg_temp_new_i32();
+    gen_helper_tosingle(tmp, src);
+    tcg_gen_qemu_st_i32(tmp, addr, ctx->mem_idx, DEF_MEMOP(MO_UL));
+    tcg_temp_free_i32(tmp);
 }
 
 /* stfd stfdu stfdux stfdx */
-- 
2.17.1

Re: [Qemu-devel] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Programmingkid]

> On Jul 3, 2018, at 11:17 AM, Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> Memory operations have no side effects on fp state.
> The use of a "real" conversions between float64 and float32
> would raise exceptions for SNaN and out-of-range inputs.

Would you have any documentation that tells us about converting
between 64 bit and 32 bit floating points?


> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/ppc/helper.h                |  4 +-
> target/ppc/fpu_helper.c            | 63 ++++++++++++++++++++++++------
> target/ppc/translate/fp-impl.inc.c | 26 +++++-------
> 3 files changed, 62 insertions(+), 31 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index cc3d031407..33e6e1df60 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -61,8 +61,8 @@ DEF_HELPER_2(compute_fprf_float64, void, env, i64)
> DEF_HELPER_3(store_fpscr, void, env, i64, i32)
> DEF_HELPER_2(fpscr_clrbit, void, env, i32)
> DEF_HELPER_2(fpscr_setbit, void, env, i32)
> -DEF_HELPER_2(float64_to_float32, i32, env, i64)
> -DEF_HELPER_2(float32_to_float64, i64, env, i32)
> +DEF_HELPER_FLAGS_1(todouble, TCG_CALL_NO_RWG_SE, i64, i32)
> +DEF_HELPER_FLAGS_1(tosingle, TCG_CALL_NO_RWG_SE, i32, i64)
> 
> DEF_HELPER_4(fcmpo, void, env, i64, i64, i32)
> DEF_HELPER_4(fcmpu, void, env, i64, i64, i32)
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 1e195487d3..d4e9e3bccb 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -47,24 +47,61 @@ static inline bool fp_exceptions_enabled(CPUPPCState *env)
> 
> /*****************************************************************************/
> /* Floating point operations helpers */
> -uint64_t helper_float32_to_float64(CPUPPCState *env, uint32_t arg)
> -{
> -    CPU_FloatU f;
> -    CPU_DoubleU d;
> 
> -    f.l = arg;
> -    d.d = float32_to_float64(f.f, &env->fp_status);
> -    return d.ll;
> +/*
> + * This is the non-arithmatic conversion that happens e.g. on loads.
> + * In the Power ISA pseudocode, this is called DOUBLE.
> + */
> +uint64_t helper_todouble(uint32_t arg)
> +{
> +    uint32_t abs_arg = arg & 0x7fffffff;
> +    uint64_t ret;
> +
> +    if (likely(abs_arg >= 0x00800000)) {
> +        /* Normalized operand, or Inf, or NaN.  */
> +        ret  = (uint64_t)extract32(arg, 30, 2) << 62;
> +        ret |= ((extract32(arg, 30, 1) ^ 1) * (uint64_t)7) << 59;
> +        ret |= (uint64_t)extract32(arg, 0, 29) << 29;
> +    } else {
> +        /* Zero or Denormalized operand.  */
> +        ret = (uint64_t)extract32(arg, 31, 1) << 63;
> +        if (unlikely(abs_arg != 0)) {
> +            /* Denormalized operand.  */
> +            int shift = clz32(abs_arg) - 9;
> +            int exp = -126 - shift + 1023;
> +            ret |= (uint64_t)exp << 52;
> +            ret |= abs_arg << (shift + 29);
> +        }
> +    }
> +    return ret;
> }
> 
> -uint32_t helper_float64_to_float32(CPUPPCState *env, uint64_t arg)
> +/*
> + * This is the non-arithmatic conversion that happens e.g. on stores.
> + * In the Power ISA pseudocode, this is called SINGLE.
> + */
> +uint32_t helper_tosingle(uint64_t arg)
> {
> -    CPU_FloatU f;
> -    CPU_DoubleU d;
> +    int exp = extract64(arg, 52, 11);
> +    uint32_t ret;
> 
> -    d.ll = arg;
> -    f.f = float64_to_float32(d.d, &env->fp_status);
> -    return f.l;
> +    if (likely(exp > 896)) {
> +        /* No denormalization required (includes Inf, NaN).  */
> +        ret  = extract64(arg, 62, 2) << 30;
> +        ret |= extract64(arg, 29, 29);
> +    } else {
> +        /* Zero or Denormal result.  If the exponent is in bounds for
> +         * a single-precision denormal result, extract the proper bits.
> +         * If the input is not zero, and the exponent is out of bounds,
> +         * then the result is undefined; this underflows to zero.
> +         */
> +        ret = extract64(arg, 63, 1) << 63;
> +        if (unlikely(exp >= 874)) {
> +            /* Denormal result.  */
> +            ret |= ((1ULL << 52) | extract64(arg, 0, 52)) >> (896 + 30 - exp);
> +        }
> +    }
> +    return ret;
> }
> 
> static inline int ppc_float32_get_unbiased_exp(float32 f)
> diff --git a/target/ppc/translate/fp-impl.inc.c b/target/ppc/translate/fp-impl.inc.c
> index 2fbd4d4f38..a6f522b85c 100644
> --- a/target/ppc/translate/fp-impl.inc.c
> +++ b/target/ppc/translate/fp-impl.inc.c
> @@ -660,15 +660,12 @@ GEN_LDUF(name, ldop, op | 0x21, type);                                        \
> GEN_LDUXF(name, ldop, op | 0x01, type);                                       \
> GEN_LDXF(name, ldop, 0x17, op | 0x00, type)
> 
> -static inline void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 arg1, TCGv arg2)
> +static void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 dest, TCGv addr)
> {
> -    TCGv t0 = tcg_temp_new();
> -    TCGv_i32 t1 = tcg_temp_new_i32();
> -    gen_qemu_ld32u(ctx, t0, arg2);
> -    tcg_gen_trunc_tl_i32(t1, t0);
> -    tcg_temp_free(t0);
> -    gen_helper_float32_to_float64(arg1, cpu_env, t1);
> -    tcg_temp_free_i32(t1);
> +    TCGv_i32 tmp = tcg_temp_new_i32();
> +    tcg_gen_qemu_ld_i32(tmp, addr, ctx->mem_idx, DEF_MEMOP(MO_UL));
> +    gen_helper_todouble(dest, tmp);
> +    tcg_temp_free_i32(tmp);
> }
> 
>  /* lfd lfdu lfdux lfdx */
> @@ -836,15 +833,12 @@ GEN_STUF(name, stop, op | 0x21, type);                                        \
> GEN_STUXF(name, stop, op | 0x01, type);                                       \
> GEN_STXF(name, stop, 0x17, op | 0x00, type)
> 
> -static inline void gen_qemu_st32fs(DisasContext *ctx, TCGv_i64 arg1, TCGv arg2)
> +static void gen_qemu_st32fs(DisasContext *ctx, TCGv_i64 src, TCGv addr)
> {
> -    TCGv_i32 t0 = tcg_temp_new_i32();
> -    TCGv t1 = tcg_temp_new();
> -    gen_helper_float64_to_float32(t0, cpu_env, arg1);
> -    tcg_gen_extu_i32_tl(t1, t0);
> -    tcg_temp_free_i32(t0);
> -    gen_qemu_st32(ctx, t1, arg2);
> -    tcg_temp_free(t1);
> +    TCGv_i32 tmp = tcg_temp_new_i32();
> +    gen_helper_tosingle(tmp, src);
> +    tcg_gen_qemu_st_i32(tmp, addr, ctx->mem_idx, DEF_MEMOP(MO_UL));
> +    tcg_temp_free_i32(tmp);
> }
> 
> /* stfd stfdu stfdux stfdx */
> -- 
> 2.17.1
> 

Re: [Qemu-devel] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Richard Henderson]

On 07/05/2018 09:31 AM, Programmingkid wrote:
>> On Jul 3, 2018, at 11:17 AM, Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> Memory operations have no side effects on fp state.
>> The use of a "real" conversions between float64 and float32
>> would raise exceptions for SNaN and out-of-range inputs.
> 
> Would you have any documentation that tells us about converting
> between 64 bit and 32 bit floating points?

Spelled out right at the beginning of sections 4.6 (load) and 4.7 (store) of
Book 1 of the Power ISA manual (version 3.0B) [0].

I've double-checked vs RISU[1] testing of LFS and STFS, with master traces
generated on Power 8 ppc64le, so I don't see anything immediately wrong with
the patch.  But I haven't had time to look further than that.


r~


[0] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
[1] https://git.linaro.org/people/peter.maydell/risu.git

Re: [Qemu-devel] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Programmingkid]

> On Jul 5, 2018, at 12:48 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 07/05/2018 09:31 AM, Programmingkid wrote:
>>> On Jul 3, 2018, at 11:17 AM, Richard Henderson <richard.henderson@linaro.org> wrote:
>>> 
>>> Memory operations have no side effects on fp state.
>>> The use of a "real" conversions between float64 and float32
>>> would raise exceptions for SNaN and out-of-range inputs.
>> 
>> Would you have any documentation that tells us about converting
>> between 64 bit and 32 bit floating points?
> 
> Spelled out right at the beginning of sections 4.6 (load) and 4.7 (store) of
> Book 1 of the Power ISA manual (version 3.0B) [0].
> 
> I've double-checked vs RISU[1] testing of LFS and STFS, with master traces
> generated on Power 8 ppc64le, so I don't see anything immediately wrong with
> the patch.  But I haven't had time to look further than that.
> 
> 
> r~
> 
> 
> [0] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
> [1] https://git.linaro.org/people/peter.maydell/risu.git

Thank you for the documentation. My guess is there are differences between the PowerPC and Power 8 implementations. PowerPC is big endian. Would you be able to do your testing again with your Power 8 CPU in big endian mode?

Re: [Qemu-devel] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

On Thu, Jul 05, 2018 at 12:51:00PM -0400, Programmingkid wrote:
> 
> > On Jul 5, 2018, at 12:48 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
> > 
> > On 07/05/2018 09:31 AM, Programmingkid wrote:
> >>> On Jul 3, 2018, at 11:17 AM, Richard Henderson <richard.henderson@linaro.org> wrote:
> >>> 
> >>> Memory operations have no side effects on fp state.
> >>> The use of a "real" conversions between float64 and float32
> >>> would raise exceptions for SNaN and out-of-range inputs.
> >> 
> >> Would you have any documentation that tells us about converting
> >> between 64 bit and 32 bit floating points?
> > 
> > Spelled out right at the beginning of sections 4.6 (load) and 4.7 (store) of
> > Book 1 of the Power ISA manual (version 3.0B) [0].
> > 
> > I've double-checked vs RISU[1] testing of LFS and STFS, with master traces
> > generated on Power 8 ppc64le, so I don't see anything immediately wrong with
> > the patch.  But I haven't had time to look further than that.
> > 
> > 
> > r~
> > 
> > 
> > [0] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0
> > [1] https://git.linaro.org/people/peter.maydell/risu.git
> 
> Thank you for the documentation. My guess is there are differences
> between the PowerPC and Power 8 implementations.

That seems very, very unlikely to me.

> PowerPC is big
> endian. Would you be able to do your testing again with your Power 8
> CPU in big endian mode?
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Mark Cave-Ayland]

[-- Attachment #1: Type: text/plain, Size: 4435 bytes --]

On 05/07/18 17:48, Richard Henderson wrote:

> On 07/05/2018 09:31 AM, Programmingkid wrote:
>>> On Jul 3, 2018, at 11:17 AM, Richard Henderson <richard.henderson@linaro.org> wrote:
>>>
>>> Memory operations have no side effects on fp state.
>>> The use of a "real" conversions between float64 and float32
>>> would raise exceptions for SNaN and out-of-range inputs.
>>
>> Would you have any documentation that tells us about converting
>> between 64 bit and 32 bit floating points?
> 
> Spelled out right at the beginning of sections 4.6 (load) and 4.7 (store) of
> Book 1 of the Power ISA manual (version 3.0B) [0].
> 
> I've double-checked vs RISU[1] testing of LFS and STFS, with master traces
> generated on Power 8 ppc64le, so I don't see anything immediately wrong with
> the patch.  But I haven't had time to look further than that.

I've had a quick look at this with the attached patch to compare the 
helper results before your patch and after, writing any differences to 
the console.

With this patch applied to ppc-for-3.1 I've booted MacOS 9 and recorded 
the output below:


$ ./qemu-system-ppc -cdrom MacOS921-macsbug.iso -boot d -M mac99

helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
3bf0000000000000
helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
3bf0000000000000

(note: MacOS 9 will hang here unless the line marked "Uncommenting this 
allows MacOS to run" in my patch is enabled)

helper_todouble diff for arg: 3f000000  d.ll: 3fe0000000000000  ret: 
3be0000000000000
helper_todouble diff for arg: 3f000000  d.ll: 3fe0000000000000  ret: 
3be0000000000000
helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
3bf0000000000000
helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
3bf0000000000000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_todouble diff for arg: be61b08a  d.ll: bfcc361140000000  ret: 
bbcc361140000000
helper_todouble diff for arg: 3fdf81a5  d.ll: 3ffbf034a0000000  ret: 
3bfbf034a0000000
helper_todouble diff for arg: bf402647  d.ll: bfe804c8e0000000  ret: 
bbe804c8e0000000
helper_todouble diff for arg: 3e61b08a  d.ll: 3fcc361140000000  ret: 
3bcc361140000000
helper_tosingle diff for arg: bfcc361140000000  f.l: be61b08a  ret: 9e61b08a
helper_todouble diff for arg: 3f0ccccd  d.ll: 3fe19999a0000000  ret: 
3be19999a0000000
helper_tosingle diff for arg: 3ffbf034a0000000  f.l: 3fdf81a5  ret: 1fdf81a5
helper_tosingle diff for arg: bfe804c8e0000000  f.l: bf402647  ret: 9f402647
helper_tosingle diff for arg: 3fcc361140000000  f.l: 3e61b08a  ret: 1e61b08a
helper_tosingle diff for arg: 3fe19999a0000000  f.l: 3f0ccccd  ret: 1f0ccccd
helper_todouble diff for arg: 3b800000  d.ll: 3f70000000000000  ret: 
3b70000000000000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_todouble diff for arg: 3b800000  d.ll: 3f70000000000000  ret: 
3b70000000000000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000


It looks like the differences are related to a flag or flags in the MSB 
byte of ret.


ATB,

Mark.

[-- Attachment #2: qemu-ppc-fp-helper-compare.patch --]
[-- Type: text/x-patch, Size: 3522 bytes --]

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index cb82e6e842..aedf5d1e3b 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -52,10 +52,15 @@ static inline bool fp_exceptions_enabled(CPUPPCState *env)
  * This is the non-arithmatic conversion that happens e.g. on loads.
  * In the Power ISA pseudocode, this is called DOUBLE.
  */
-uint64_t helper_todouble(uint32_t arg)
+uint64_t helper_todouble(CPUPPCState *env, uint32_t arg)
 {
     uint32_t abs_arg = arg & 0x7fffffff;
     uint64_t ret;
+    CPU_FloatU f;
+    CPU_DoubleU d;
+
+    f.l = arg;
+    d.d = float32_to_float64(f.f, &env->fp_status);
 
     if (likely(abs_arg >= 0x00800000)) {
         /* Normalized operand, or Inf, or NaN.  */
@@ -73,6 +78,13 @@ uint64_t helper_todouble(uint32_t arg)
             ret |= abs_arg << (shift + 29);
         }
     }
+
+    if (d.ll != ret) {
+        printf("helper_todouble diff for arg: %x  d.ll: %lx  ret: %lx\n", arg, d.ll, ret);
+        // Uncommenting this allows MacOS to run
+        //ret = d.ll;
+    }
+
     return ret;
 }
 
@@ -80,10 +92,15 @@ uint64_t helper_todouble(uint32_t arg)
  * This is the non-arithmatic conversion that happens e.g. on stores.
  * In the Power ISA pseudocode, this is called SINGLE.
  */
-uint32_t helper_tosingle(uint64_t arg)
+uint32_t helper_tosingle(CPUPPCState *env, uint64_t arg)
 {
     int exp = extract64(arg, 52, 11);
     uint32_t ret;
+    CPU_FloatU f;
+    CPU_DoubleU d;
+
+    d.ll = arg;
+    f.f = float64_to_float32(d.d, &env->fp_status);
 
     if (likely(exp > 896)) {
         /* No denormalization required (includes Inf, NaN).  */
@@ -101,6 +118,11 @@ uint32_t helper_tosingle(uint64_t arg)
             ret |= ((1ULL << 52) | extract64(arg, 0, 52)) >> (896 + 30 - exp);
         }
     }
+
+    if (f.l != ret) {
+        printf("helper_tosingle diff for arg: %lx  f.l: %x  ret: %x\n", arg, f.l, ret);
+    }
+
     return ret;
 }
 
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index ef64248bc4..94b5f4fd8c 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -61,8 +61,8 @@ DEF_HELPER_2(compute_fprf_float64, void, env, i64)
 DEF_HELPER_3(store_fpscr, void, env, i64, i32)
 DEF_HELPER_2(fpscr_clrbit, void, env, i32)
 DEF_HELPER_2(fpscr_setbit, void, env, i32)
-DEF_HELPER_FLAGS_1(todouble, TCG_CALL_NO_RWG_SE, i64, i32)
-DEF_HELPER_FLAGS_1(tosingle, TCG_CALL_NO_RWG_SE, i32, i64)
+DEF_HELPER_FLAGS_2(todouble, TCG_CALL_NO_RWG_SE, i64, env, i32)
+DEF_HELPER_FLAGS_2(tosingle, TCG_CALL_NO_RWG_SE, i32, env, i64)
 
 DEF_HELPER_4(fcmpo, void, env, i64, i64, i32)
 DEF_HELPER_4(fcmpu, void, env, i64, i64, i32)
diff --git a/target/ppc/translate/fp-impl.inc.c b/target/ppc/translate/fp-impl.inc.c
index a6f522b85c..16ecca3048 100644
--- a/target/ppc/translate/fp-impl.inc.c
+++ b/target/ppc/translate/fp-impl.inc.c
@@ -664,7 +664,7 @@ static void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 dest, TCGv addr)
 {
     TCGv_i32 tmp = tcg_temp_new_i32();
     tcg_gen_qemu_ld_i32(tmp, addr, ctx->mem_idx, DEF_MEMOP(MO_UL));
-    gen_helper_todouble(dest, tmp);
+    gen_helper_todouble(dest, cpu_env, tmp);
     tcg_temp_free_i32(tmp);
 }
 
@@ -836,7 +836,7 @@ GEN_STXF(name, stop, 0x17, op | 0x00, type)
 static void gen_qemu_st32fs(DisasContext *ctx, TCGv_i64 src, TCGv addr)
 {
     TCGv_i32 tmp = tcg_temp_new_i32();
-    gen_helper_tosingle(tmp, src);
+    gen_helper_tosingle(tmp, cpu_env, src);
     tcg_gen_qemu_st_i32(tmp, addr, ctx->mem_idx, DEF_MEMOP(MO_UL));
     tcg_temp_free_i32(tmp);
 }

Re: [Qemu-devel] [Qemu-ppc] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Mark Cave-Ayland]

On 06/07/18 10:03, Mark Cave-Ayland wrote:

> On 05/07/18 17:48, Richard Henderson wrote:
> 
>> On 07/05/2018 09:31 AM, Programmingkid wrote:
>>>> On Jul 3, 2018, at 11:17 AM, Richard Henderson 
>>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> Memory operations have no side effects on fp state.
>>>> The use of a "real" conversions between float64 and float32
>>>> would raise exceptions for SNaN and out-of-range inputs.
>>>
>>> Would you have any documentation that tells us about converting
>>> between 64 bit and 32 bit floating points?
>>
>> Spelled out right at the beginning of sections 4.6 (load) and 4.7 
>> (store) of
>> Book 1 of the Power ISA manual (version 3.0B) [0].
>>
>> I've double-checked vs RISU[1] testing of LFS and STFS, with master 
>> traces
>> generated on Power 8 ppc64le, so I don't see anything immediately 
>> wrong with
>> the patch.  But I haven't had time to look further than that.
> 
> I've had a quick look at this with the attached patch to compare the 
> helper results before your patch and after, writing any differences to 
> the console.
> 
> With this patch applied to ppc-for-3.1 I've booted MacOS 9 and recorded 
> the output below:
> 
> 
> $ ./qemu-system-ppc -cdrom MacOS921-macsbug.iso -boot d -M mac99
> 
> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
> 3bf0000000000000
> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
> 3bf0000000000000
> 
> (note: MacOS 9 will hang here unless the line marked "Uncommenting this 
> allows MacOS to run" in my patch is enabled)
> 
> helper_todouble diff for arg: 3f000000  d.ll: 3fe0000000000000  ret: 
> 3be0000000000000
> helper_todouble diff for arg: 3f000000  d.ll: 3fe0000000000000  ret: 
> 3be0000000000000
> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
> 3bf0000000000000
> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret: 
> 3bf0000000000000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_todouble diff for arg: be61b08a  d.ll: bfcc361140000000  ret: 
> bbcc361140000000
> helper_todouble diff for arg: 3fdf81a5  d.ll: 3ffbf034a0000000  ret: 
> 3bfbf034a0000000
> helper_todouble diff for arg: bf402647  d.ll: bfe804c8e0000000  ret: 
> bbe804c8e0000000
> helper_todouble diff for arg: 3e61b08a  d.ll: 3fcc361140000000  ret: 
> 3bcc361140000000
> helper_tosingle diff for arg: bfcc361140000000  f.l: be61b08a  ret: 
> 9e61b08a
> helper_todouble diff for arg: 3f0ccccd  d.ll: 3fe19999a0000000  ret: 
> 3be19999a0000000
> helper_tosingle diff for arg: 3ffbf034a0000000  f.l: 3fdf81a5  ret: 
> 1fdf81a5
> helper_tosingle diff for arg: bfe804c8e0000000  f.l: bf402647  ret: 
> 9f402647
> helper_tosingle diff for arg: 3fcc361140000000  f.l: 3e61b08a  ret: 
> 1e61b08a
> helper_tosingle diff for arg: 3fe19999a0000000  f.l: 3f0ccccd  ret: 
> 1f0ccccd
> helper_todouble diff for arg: 3b800000  d.ll: 3f70000000000000  ret: 
> 3b70000000000000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_todouble diff for arg: 3b800000  d.ll: 3f70000000000000  ret: 
> 3b70000000000000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 
> 1f800000
> 
> 
> It looks like the differences are related to a flag or flags in the MSB 
> byte of ret.

Hi Richard,

Have you had a chance to look at this yet? I've been working on top of 
David's ppc-for-3.1 branch over the weekend and ran into this again 
during my testing :/


ATB,

Mark.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 7/7] target/ppc: Use non-arithmetic conversions for fp load/store

[Richard Henderson]

On 08/05/2018 04:41 AM, Mark Cave-Ayland wrote:
> On 06/07/18 10:03, Mark Cave-Ayland wrote:
> 
>> On 05/07/18 17:48, Richard Henderson wrote:
>>
>>> On 07/05/2018 09:31 AM, Programmingkid wrote:
>>>>> On Jul 3, 2018, at 11:17 AM, Richard Henderson
>>>>> <richard.henderson@linaro.org> wrote:
>>>>>
>>>>> Memory operations have no side effects on fp state.
>>>>> The use of a "real" conversions between float64 and float32
>>>>> would raise exceptions for SNaN and out-of-range inputs.
>>>>
>>>> Would you have any documentation that tells us about converting
>>>> between 64 bit and 32 bit floating points?
>>>
>>> Spelled out right at the beginning of sections 4.6 (load) and 4.7 (store) of
>>> Book 1 of the Power ISA manual (version 3.0B) [0].
>>>
>>> I've double-checked vs RISU[1] testing of LFS and STFS, with master traces
>>> generated on Power 8 ppc64le, so I don't see anything immediately wrong with
>>> the patch.  But I haven't had time to look further than that.
>>
>> I've had a quick look at this with the attached patch to compare the helper
>> results before your patch and after, writing any differences to the console.
>>
>> With this patch applied to ppc-for-3.1 I've booted MacOS 9 and recorded the
>> output below:
>>
>>
>> $ ./qemu-system-ppc -cdrom MacOS921-macsbug.iso -boot d -M mac99
>>
>> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret:
>> 3bf0000000000000
>> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret:
>> 3bf0000000000000
>>
>> (note: MacOS 9 will hang here unless the line marked "Uncommenting this
>> allows MacOS to run" in my patch is enabled)
>>
>> helper_todouble diff for arg: 3f000000  d.ll: 3fe0000000000000  ret:
>> 3be0000000000000
>> helper_todouble diff for arg: 3f000000  d.ll: 3fe0000000000000  ret:
>> 3be0000000000000
>> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret:
>> 3bf0000000000000
>> helper_todouble diff for arg: 3f800000  d.ll: 3ff0000000000000  ret:
>> 3bf0000000000000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_todouble diff for arg: be61b08a  d.ll: bfcc361140000000  ret:
>> bbcc361140000000
>> helper_todouble diff for arg: 3fdf81a5  d.ll: 3ffbf034a0000000  ret:
>> 3bfbf034a0000000
>> helper_todouble diff for arg: bf402647  d.ll: bfe804c8e0000000  ret:
>> bbe804c8e0000000
>> helper_todouble diff for arg: 3e61b08a  d.ll: 3fcc361140000000  ret:
>> 3bcc361140000000
>> helper_tosingle diff for arg: bfcc361140000000  f.l: be61b08a  ret: 9e61b08a
>> helper_todouble diff for arg: 3f0ccccd  d.ll: 3fe19999a0000000  ret:
>> 3be19999a0000000
>> helper_tosingle diff for arg: 3ffbf034a0000000  f.l: 3fdf81a5  ret: 1fdf81a5
>> helper_tosingle diff for arg: bfe804c8e0000000  f.l: bf402647  ret: 9f402647
>> helper_tosingle diff for arg: 3fcc361140000000  f.l: 3e61b08a  ret: 1e61b08a
>> helper_tosingle diff for arg: 3fe19999a0000000  f.l: 3f0ccccd  ret: 1f0ccccd
>> helper_todouble diff for arg: 3b800000  d.ll: 3f70000000000000  ret:
>> 3b70000000000000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_todouble diff for arg: 3b800000  d.ll: 3f70000000000000  ret:
>> 3b70000000000000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>> helper_tosingle diff for arg: 3ff0000000000000  f.l: 3f800000  ret: 1f800000
>>
>>
>> It looks like the differences are related to a flag or flags in the MSB byte
>> of ret.
> 
> Hi Richard,
> 
> Have you had a chance to look at this yet? I've been working on top of David's
> ppc-for-3.1 branch over the weekend and ran into this again during my testing :/

Thanks for the reminder and the test cases.
I've posted a fix for this now.


r~

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

On Tue, Jul 03, 2018 at 08:17:25AM -0700, Richard Henderson wrote:
> Beginning with John Arbuckle's fdiv test case, clean up some
> of the fp helpers.  As with fdiv, fre and fresqrt are missing
> divide-by-zero exceptions.
> 
> I've also noticed that load/store were using arithmetic conversions
> to/from float32.  These should be using the non-arithmetic algorithms
> listed in the manual.
> 
> Aside from cleaning up all of the rest of the helpers, I believe that
> the implementation of the single-precision operations are incorrect.
> They are currently implemented with the double-precision operation
> followed by a round-to-single.  This causes incorrect results via
> double rounding.  I believe better results could be had by using
> these non-arithmetic converters to produce float32 operands, use the
> proper float32 softfloat operations, and then convert back.
> 
> Anyway, all of this has been broken long enough that it'll need to
> wait til next devel cycle before anything further gets done.

Applied to ppc-for-3.1 (newly created).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Programmingkid]

> On Jul 3, 2018, at 11:31 PM, David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Tue, Jul 03, 2018 at 08:17:25AM -0700, Richard Henderson wrote:
>> Beginning with John Arbuckle's fdiv test case, clean up some
>> of the fp helpers.  As with fdiv, fre and fresqrt are missing
>> divide-by-zero exceptions.
>> 
>> I've also noticed that load/store were using arithmetic conversions
>> to/from float32.  These should be using the non-arithmetic algorithms
>> listed in the manual.
>> 
>> Aside from cleaning up all of the rest of the helpers, I believe that
>> the implementation of the single-precision operations are incorrect.
>> They are currently implemented with the double-precision operation
>> followed by a round-to-single.  This causes incorrect results via
>> double rounding.  I believe better results could be had by using
>> these non-arithmetic converters to produce float32 operands, use the
>> proper float32 softfloat operations, and then convert back.
>> 
>> Anyway, all of this has been broken long enough that it'll need to
>> wait til next devel cycle before anything further gets done.
> 
> Applied to ppc-for-3.1 (newly created)

Could I have the address for your ppc-for-3.1 repo please? Would you like future floating point patches to be based on this repo?

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On Wed, Jul 04, 2018 at 09:42:16AM -0400, Programmingkid wrote:
> 
> > On Jul 3, 2018, at 11:31 PM, David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > On Tue, Jul 03, 2018 at 08:17:25AM -0700, Richard Henderson wrote:
> >> Beginning with John Arbuckle's fdiv test case, clean up some
> >> of the fp helpers.  As with fdiv, fre and fresqrt are missing
> >> divide-by-zero exceptions.
> >> 
> >> I've also noticed that load/store were using arithmetic conversions
> >> to/from float32.  These should be using the non-arithmetic algorithms
> >> listed in the manual.
> >> 
> >> Aside from cleaning up all of the rest of the helpers, I believe that
> >> the implementation of the single-precision operations are incorrect.
> >> They are currently implemented with the double-precision operation
> >> followed by a round-to-single.  This causes incorrect results via
> >> double rounding.  I believe better results could be had by using
> >> these non-arithmetic converters to produce float32 operands, use the
> >> proper float32 softfloat operations, and then convert back.
> >> 
> >> Anyway, all of this has been broken long enough that it'll need to
> >> wait til next devel cycle before anything further gets done.
> > 
> > Applied to ppc-for-3.1 (newly created)
> 
> Could I have the address for your ppc-for-3.1 repo please? Would you
> like future floating point patches to be based on this repo?

See https://github.com/dgibson/qemu/tree/ppc-for-3.1

Same repo as my existing ppc-for-3.0 tree, different branch.

And yes, patches should be based on this tree, unless they're
important bugfixes that are aimed at 3.0.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

>
> Anyway, all of this has been broken long enough that it'll need to
> wait til next devel cycle before anything further gets done.

When it gets done should we add some explicit ppc tests to tests/tcg/ppc
like we did with arm64 fcvt or would it be worth spending more time on
the general purpose FP test suite Emilio was working on as part of his
hard-float series?

>
>
> r~
>
>
> Richard Henderson (7):
>   target/ppc: Enable fp exceptions for user-only
>   target/ppc: Honor fpscr_ze semantics and tidy fdiv
>   target/ppc: Tidy helper_fmul
>   target/ppc: Tidy helper_fadd, helper_fsub
>   target/ppc: Tidy helper_fsqrt
>   target/ppc: Honor fpscr_ze semantics and tidy fre, fresqrt
>   target/ppc: Use non-arithmetic conversions for fp load/store
>
>  target/ppc/helper.h                |  14 +-
>  target/ppc/fpu_helper.c            | 294 +++++++++++++++++------------
>  target/ppc/translate/fp-impl.inc.c |  26 +--
>  target/ppc/translate_init.inc.c    |   2 +
>  4 files changed, 189 insertions(+), 147 deletions(-)


--
Alex Bennée

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Programmingkid]

> On Jul 4, 2018, at 10:15 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> 
>> Anyway, all of this has been broken long enough that it'll need to
>> wait til next devel cycle before anything further gets done.
> 
> When it gets done should we add some explicit ppc tests to tests/tcg/ppc
> like we did with arm64 fcvt or would it be worth spending more time on
> the general purpose FP test suite Emilio was working on as part of his
> hard-float series?

My vote goes to adding PowerPC specific tests. I already made some and sent them to the list a couple of days ago. A general purpose floating point test suite would probably not be able to test things like PowerPC floating point flags. 

I'm not certain how we would add floating point tests to QEMU. I'm guessing it would involve some kind of bootable image file that would load and run the tests. Hopefully there is an easier way to implement testing.

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Alex Bennée]

Programmingkid <programmingkidx@gmail.com> writes:

>> On Jul 4, 2018, at 10:15 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>>
>>> Anyway, all of this has been broken long enough that it'll need to
>>> wait til next devel cycle before anything further gets done.
>>
>> When it gets done should we add some explicit ppc tests to tests/tcg/ppc
>> like we did with arm64 fcvt or would it be worth spending more time on
>> the general purpose FP test suite Emilio was working on as part of his
>> hard-float series?
>
> My vote goes to adding PowerPC specific tests. I already made some and sent them to the list a couple of days ago. A general purpose floating point test suite would probably not be able to test things like PowerPC floating point flags.
>
> I'm not certain how we would add floating point tests to QEMU. I'm
> guessing it would involve some kind of bootable image file that would
> load and run the tests. Hopefully there is an easier way to implement
> testing.

You don't need system tests - this is all testable from linux-user. See
tests/tcg/arm/fcvt.c and the .ref files in arm and aarch64 directories.
Assuming the test isn't insane (like test-i386-fprem) we can just add a
reference output recorded on know good hardware.

--
Alex Bennée

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Programmingkid]

> On Jul 4, 2018, at 11:39 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> 
> Programmingkid <programmingkidx@gmail.com> writes:
> 
>>> On Jul 4, 2018, at 10:15 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> 
>>> 
>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>> 
>>>> 
>>>> Anyway, all of this has been broken long enough that it'll need to
>>>> wait til next devel cycle before anything further gets done.
>>> 
>>> When it gets done should we add some explicit ppc tests to tests/tcg/ppc
>>> like we did with arm64 fcvt or would it be worth spending more time on
>>> the general purpose FP test suite Emilio was working on as part of his
>>> hard-float series?
>> 
>> My vote goes to adding PowerPC specific tests. I already made some and sent them to the list a couple of days ago. A general purpose floating point test suite would probably not be able to test things like PowerPC floating point flags.
>> 
>> I'm not certain how we would add floating point tests to QEMU. I'm
>> guessing it would involve some kind of bootable image file that would
>> load and run the tests. Hopefully there is an easier way to implement
>> testing.
> 
> You don't need system tests - this is all testable from linux-user. See
> tests/tcg/arm/fcvt.c and the .ref files in arm and aarch64 directories.
> Assuming the test isn't insane (like test-i386-fprem) we can just add a
> reference output recorded on know good hardware.

My hope is to make testing the floating point unit as easy as 'make test' or maybe 'make fp-test'. 

Re: [Qemu-devel] [PATCH for-3.1 0/7] target/ppc fp cleanups

[Alex Bennée]

Programmingkid <programmingkidx@gmail.com> writes:

>> On Jul 4, 2018, at 11:39 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Programmingkid <programmingkidx@gmail.com> writes:
>>
>>>> On Jul 4, 2018, at 10:15 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>>
>>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>>
>>>>>
>>>>> Anyway, all of this has been broken long enough that it'll need to
>>>>> wait til next devel cycle before anything further gets done.
>>>>
>>>> When it gets done should we add some explicit ppc tests to tests/tcg/ppc
>>>> like we did with arm64 fcvt or would it be worth spending more time on
>>>> the general purpose FP test suite Emilio was working on as part of his
>>>> hard-float series?
>>>
>>> My vote goes to adding PowerPC specific tests. I already made some and sent them to the list a couple of days ago. A general purpose floating point test suite would probably not be able to test things like PowerPC floating point flags.
>>>
>>> I'm not certain how we would add floating point tests to QEMU. I'm
>>> guessing it would involve some kind of bootable image file that would
>>> load and run the tests. Hopefully there is an easier way to implement
>>> testing.
>>
>> You don't need system tests - this is all testable from linux-user. See
>> tests/tcg/arm/fcvt.c and the .ref files in arm and aarch64 directories.
>> Assuming the test isn't insane (like test-i386-fprem) we can just add a
>> reference output recorded on know good hardware.
>
> My hope is to make testing the floating point unit as easy as 'make
> test' or maybe 'make fp-test'.

That would be "make check-tcg"


--
Alex Bennée