[RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[Lucas Mateus Castro(alqotel)]

From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>

As mentioned in the functions float_overflow_excp and
float_underflow_excp, the result should be adjusted as mentioned in the
ISA (subtracted 192/1536 from the exponent of the intermediate result if
an overflow occurs with OE set and added 192/1536 to the exponent of the
intermediate result if an underflow occurs with UE set), but at those
functions the result has already been rounded so it is not possible to
add/subtract from the intermediate result anymore.
 
This patch creates a new function that receives the value that should be
subtracted/added from the exponent if an overflow/underflow happens, to
not leave some arbitrary numbers from the PowerISA in the middle of the
FPU code. If these numbers are 0 the new functions just call the old
ones.

I used 2 values here for overflow and underflow, maybe it'd be better to
just use the same ones, any thoughts?

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
An alternative I've thought was to always return the value adjusted if a
overflow or underflow occurs and in float_underflow_excp and
float_overflow_excp adjust it to inf/den/0 if OE/UE is 0, but I didn't
saw many advantages to that approach.
---
 fpu/softfloat.c         | 75 +++++++++++++++++++++++++++++++++++++++++
 include/fpu/softfloat.h |  2 ++
 target/ppc/fpu_helper.c | 10 ++++--
 3 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 4a871ef2a1..a407129dcb 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -268,6 +268,8 @@ typedef bool (*f64_check_fn)(union_float64 a, union_float64 b);
 
 typedef float32 (*soft_f32_op2_fn)(float32 a, float32 b, float_status *s);
 typedef float64 (*soft_f64_op2_fn)(float64 a, float64 b, float_status *s);
+typedef float64 (*soft_f64_op2_int2_fn)(float64 a, float64 b, int c, int d,
+                                        float_status *s);
 typedef float   (*hard_f32_op2_fn)(float a, float b);
 typedef double  (*hard_f64_op2_fn)(double a, double b);
 
@@ -401,6 +403,19 @@ float64_gen2(float64 xa, float64 xb, float_status *s,
     return soft(ua.s, ub.s, s);
 }
 
+static inline float64
+float64_gen2_excp(float64 xa, float64 xb, int xc, int xd, float_status *s,
+                  hard_f64_op2_fn hard, soft_f64_op2_fn soft,
+                  soft_f64_op2_int2_fn soft_excp, f64_check_fn pre,
+                  f64_check_fn post)
+{
+    if (xc || xd) {
+        return soft_excp(xa, xb, xc, xd, s);
+    } else {
+        return float64_gen2(xa, xb, s, hard, soft, pre, post);
+    }
+}
+
 /*
  * Classify a floating point number. Everything above float_class_qnan
  * is a NaN so cls >= float_class_qnan is any NaN.
@@ -1929,6 +1944,39 @@ static double hard_f64_sub(double a, double b)
     return a - b;
 }
 
+static float64 QEMU_SOFTFLOAT_ATTR
+soft_f64_addsub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                        float_status *status, bool subtract)
+{
+    FloatParts64 pa, pb, *pr;
+
+    float64_unpack_canonical(&pa, a, status);
+    float64_unpack_canonical(&pb, b, status);
+    pr = parts_addsub(&pa, &pb, status, subtract);
+
+    if (unlikely(oe_sub && (pr->exp > 1023))) {
+        pr->exp -= oe_sub;
+        float_raise(float_flag_overflow, status);
+    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
+        pr->exp += ue_sum;
+        float_raise(float_flag_underflow, status);
+    }
+
+    return float64_round_pack_canonical(pr, status);
+}
+
+static float64 soft_f64_add_excp_en(float64 a, float64 b, int oe_sub,
+                                    int ue_sum, float_status *status)
+{
+    return soft_f64_addsub_excp_en(a, b, oe_sub, ue_sum, status, false);
+}
+
+static float64 soft_f64_sub_excp_en(float64 a, float64 b, int oe_sub,
+                                    int ue_sum, float_status *status)
+{
+    return soft_f64_addsub_excp_en(a, b, oe_sub, ue_sum, status, true);
+}
+
 static bool f32_addsubmul_post(union_float32 a, union_float32 b)
 {
     if (QEMU_HARDFLOAT_2F32_USE_FP) {
@@ -1960,6 +2008,15 @@ static float64 float64_addsub(float64 a, float64 b, float_status *s,
                         f64_is_zon2, f64_addsubmul_post);
 }
 
+static float64 float64_addsub_excp_en(float64 a, float64 b, int oe_sum,
+                                      int ue_sub, float_status *s,
+                                      hard_f64_op2_fn hard, soft_f64_op2_fn soft,
+                                      soft_f64_op2_int2_fn soft_excp)
+{
+    return float64_gen2_excp(a, b, oe_sum, ue_sub, s, hard, soft, soft_excp,
+                             f64_is_zon2, f64_addsubmul_post);
+}
+
 float32 QEMU_FLATTEN
 float32_add(float32 a, float32 b, float_status *s)
 {
@@ -1984,6 +2041,24 @@ float64_sub(float64 a, float64 b, float_status *s)
     return float64_addsub(a, b, s, hard_f64_sub, soft_f64_sub);
 }
 
+float64 QEMU_FLATTEN
+float64_add_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                    float_status *s)
+{
+    return float64_addsub_excp_en(a, b, oe_sub, ue_sum, s,
+                                  hard_f64_add, soft_f64_add,
+                                  soft_f64_add_excp_en);
+}
+
+float64 QEMU_FLATTEN
+float64_sub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                    float_status *s)
+{
+    return float64_addsub_excp_en(a, b, oe_sub, ue_sum, s,
+                                  hard_f64_sub, soft_f64_sub,
+                                  soft_f64_sub_excp_en);
+}
+
 static float64 float64r32_addsub(float64 a, float64 b, float_status *status,
                                  bool subtract)
 {
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 3dcf20e3a2..76bf628a29 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -772,7 +772,9 @@ float128 float64_to_float128(float64, float_status *status);
 *----------------------------------------------------------------------------*/
 float64 float64_round_to_int(float64, float_status *status);
 float64 float64_add(float64, float64, float_status *status);
+float64 float64_add_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_sub(float64, float64, float_status *status);
+float64 float64_sub_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_mul(float64, float64, float_status *status);
 float64 float64_div(float64, float64, float_status *status);
 float64 float64_rem(float64, float64, float_status *status);
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 7ab6beadad..cb82c91340 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -529,7 +529,10 @@ static void float_invalid_op_addsub(CPUPPCState *env, int flags,
 /* fadd - fadd. */
 float64 helper_fadd(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    float64 ret = float64_add(arg1, arg2, &env->fp_status);
+    int oe_sub = (FP_OE & env->fpscr) ? 1536 : 0;
+    int ue_add = (FP_UE & env->fpscr) ? 1536 : 0;
+    float64 ret = float64_add_excp_en(arg1, arg2, oe_sub, ue_add,
+                                      &env->fp_status);
     int flags = get_float_exception_flags(&env->fp_status);
 
     if (unlikely(flags & float_flag_invalid)) {
@@ -554,7 +557,10 @@ float64 helper_fadds(CPUPPCState *env, float64 arg1, float64 arg2)
 /* fsub - fsub. */
 float64 helper_fsub(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    float64 ret = float64_sub(arg1, arg2, &env->fp_status);
+    int oe_sub = (FP_OE & env->fpscr) ? 1536 : 0;
+    int ue_add = (FP_UE & env->fpscr) ? 1536 : 0;
+    float64 ret = float64_sub_excp_en(arg1, arg2, oe_sub, ue_add,
+                                      &env->fp_status);
     int flags = get_float_exception_flags(&env->fp_status);
 
     if (unlikely(flags & float_flag_invalid)) {
-- 
2.31.1

[RFC PATCH 2/3] target/ppc: Bugfix fmul result with OE/UE set

[Lucas Mateus Castro(alqotel)]

From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>

Change fmul in the same way of fadd/fsub to handle overflow/underflow if
OE/UE is set (i.e. function that receives a value to add/subtract from
the exponent if an overflow/underflow occurs).

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 fpu/softfloat.c         | 30 ++++++++++++++++++++++++++++++
 include/fpu/softfloat.h |  1 +
 target/ppc/fpu_helper.c |  5 ++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index a407129dcb..e2b4ad4b63 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -2212,6 +2212,36 @@ float64_mul(float64 a, float64 b, float_status *s)
                         f64_is_zon2, f64_addsubmul_post);
 }
 
+static float64 QEMU_SOFTFLOAT_ATTR
+soft_f64_mul_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                     float_status *s)
+{
+    FloatParts64 pa, pb, *pr;
+
+    float64_unpack_canonical(&pa, a, s);
+    float64_unpack_canonical(&pb, b, s);
+    pr = parts_mul(&pa, &pb, s);
+
+    if (unlikely(oe_sub && (pr->exp > 1023))) {
+        pr->exp -= oe_sub;
+        float_raise(float_flag_overflow, s);
+    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
+        pr->exp += ue_sum;
+        float_raise(float_flag_underflow, s);
+    }
+
+    return float64_round_pack_canonical(pr, s);
+}
+
+float64 QEMU_FLATTEN
+float64_mul_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                    float_status *status)
+{
+    return float64_gen2_excp(a, b, oe_sub, ue_sum, status,
+                             hard_f64_mul, soft_f64_mul, soft_f64_mul_excp_en,
+                             f64_is_zon2, f64_addsubmul_post);
+}
+
 float64 float64r32_mul(float64 a, float64 b, float_status *status)
 {
     FloatParts64 pa, pb, *pr;
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 76bf628a29..4ff56b0e10 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -776,6 +776,7 @@ float64 float64_add_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_sub(float64, float64, float_status *status);
 float64 float64_sub_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_mul(float64, float64, float_status *status);
+float64 float64_mul_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_div(float64, float64, float_status *status);
 float64 float64_rem(float64, float64, float_status *status);
 float64 float64_muladd(float64, float64, float64, int, float_status *status);
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index cb82c91340..18cf720743 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -595,7 +595,10 @@ static void float_invalid_op_mul(CPUPPCState *env, int flags,
 /* fmul - fmul. */
 float64 helper_fmul(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    float64 ret = float64_mul(arg1, arg2, &env->fp_status);
+    int oe_sub = (FP_OE & env->fpscr) ? 1536 : 0;
+    int ue_sum = (FP_UE & env->fpscr) ? 1536 : 0;
+    float64 ret = float64_mul_excp_en(arg1, arg2, oe_sub, ue_sum,
+                                      &env->fp_status);
     int flags = get_float_exception_flags(&env->fp_status);
 
     if (unlikely(flags & float_flag_invalid)) {
-- 
2.31.1

[RFC PATCH 3/3] target/ppc: Bugfix fdiv result with OE/UE set

[Lucas Mateus Castro(alqotel)]

From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>

Change fdiv in the same way of fadd/fsub to handle overflow/underflow if
OE/UE is set (i.e. function that receives a value to add/subtract from
the exponent if an overflow/underflow occurs).

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 fpu/softfloat.c         | 30 ++++++++++++++++++++++++++++++
 include/fpu/softfloat.h |  1 +
 target/ppc/fpu_helper.c |  5 ++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index e2b4ad4b63..0e9d2d2678 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -2558,6 +2558,27 @@ soft_f64_div(float64 a, float64 b, float_status *status)
     return float64_round_pack_canonical(pr, status);
 }
 
+static float64 QEMU_SOFTFLOAT_ATTR
+soft_f64_div_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                     float_status *status)
+{
+    FloatParts64 pa, pb, *pr;
+
+    float64_unpack_canonical(&pa, a, status);
+    float64_unpack_canonical(&pb, b, status);
+    pr = parts_div(&pa, &pb, status);
+
+    if (unlikely(oe_sub && (pr->exp > 1023))) {
+        pr->exp -= oe_sub;
+        float_raise(float_flag_overflow, status);
+    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
+        pr->exp += ue_sum;
+        float_raise(float_flag_underflow, status);
+    }
+
+    return float64_round_pack_canonical(pr, status);
+}
+
 static float hard_f32_div(float a, float b)
 {
     return a / b;
@@ -2616,6 +2637,15 @@ float64_div(float64 a, float64 b, float_status *s)
                         f64_div_pre, f64_div_post);
 }
 
+float64 QEMU_FLATTEN
+float64_div_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
+                    float_status *s)
+{
+    return float64_gen2_excp(a, b, oe_sub, ue_sum, s, hard_f64_div,
+                             soft_f64_div, soft_f64_div_excp_en, f64_div_pre,
+                             f64_div_post);
+}
+
 float64 float64r32_div(float64 a, float64 b, float_status *status)
 {
     FloatParts64 pa, pb, *pr;
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 4ff56b0e10..a6c7885fcd 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -778,6 +778,7 @@ float64 float64_sub_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_mul(float64, float64, float_status *status);
 float64 float64_mul_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_div(float64, float64, float_status *status);
+float64 float64_div_excp_en(float64, float64, int, int, float_status *status);
 float64 float64_rem(float64, float64, float_status *status);
 float64 float64_muladd(float64, float64, float64, int, float_status *status);
 float64 float64_sqrt(float64, float_status *status);
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 18cf720743..1a6869a920 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -635,7 +635,10 @@ static void float_invalid_op_div(CPUPPCState *env, int flags,
 /* fdiv - fdiv. */
 float64 helper_fdiv(CPUPPCState *env, float64 arg1, float64 arg2)
 {
-    float64 ret = float64_div(arg1, arg2, &env->fp_status);
+    int oe_sub = (FP_OE & env->fpscr) ? 1536 : 0;
+    int ue_sum = (FP_UE & env->fpscr) ? 1536 : 0;
+    float64 ret = float64_div_excp_en(arg1, arg2, oe_sub, ue_sum,
+                                      &env->fp_status);
     int flags = get_float_exception_flags(&env->fp_status);
 
     if (unlikely(flags & float_flag_invalid)) {
-- 
2.31.1

[PATCH] tests/tcg/ppc64le: Added OE/UE enabled exception test

[Lucas Mateus Castro(alqotel)]

From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>

DO NOT MERGE

This patch adds a test to check if the add/sub of the intermediate
result when an overflow or underflow exception with the corresponding
enabling bit being set (i.e. OE/UE), but linux-user currently can't
disable MSR.FE0 and MSR.FE1 so it will always result in a trapping
exception, to avoid that the test should be run in a VM or use Matheus'
WIP patch in 
https://github.com/PPC64/qemu/tree/alqotel-ferst-prctl-patch

The test results were based on a Power9 machine.

Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
---
 tests/tcg/ppc64le/oe_ue_excp.c | 105 +++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 tests/tcg/ppc64le/oe_ue_excp.c

diff --git a/tests/tcg/ppc64le/oe_ue_excp.c b/tests/tcg/ppc64le/oe_ue_excp.c
new file mode 100644
index 0000000000..384219a366
--- /dev/null
+++ b/tests/tcg/ppc64le/oe_ue_excp.c
@@ -0,0 +1,105 @@
+#include <stdio.h>
+#include <float.h>
+#include <sys/prctl.h>
+
+#define FP_OE (1ull << 6)
+#define FP_UE (1ull << 5)
+
+typedef union {
+    double d;
+    long long ll;
+} ll_fp;
+
+double asm_fmul (double a, double b)
+{
+    double t;
+    asm (
+        "lfd 0, %1\n\t"
+        "lfd 1, %2\n\t"
+        "fmul 2, 0, 1\n\t"
+        "stfd 2, %0\n\t"
+        :"=m"(t)
+        :"m"(a),"m"(b)
+        );
+    return t;
+}
+
+double asm_fdiv (double a, double b)
+{
+    double t;
+    asm (
+        "lfd 0, %1\n\t"
+        "lfd 1, %2\n\t"
+        "fdiv 2, 0, 1\n\t"
+        "stfd 2, %0\n\t"
+        :"=m"(t)
+        :"m"(a),"m"(b)
+        );
+    return t;
+}
+
+int main ()
+{
+    int i, ok = 1;
+    ll_fp fpscr, t;
+
+    prctl(PR_SET_FPEXC, PR_FP_EXC_DISABLED);
+
+    fpscr.ll = FP_UE | FP_OE;
+    __builtin_mtfsf (0b11111111, fpscr.d);
+    fpscr.d = __builtin_mffs ();
+    printf("fpscr = %016llx\n", fpscr.ll);
+
+    ll_fp ch[] =
+                {
+                    { .ll = 0x1b64f1c1b0000000ull },
+                    { .ll = 0x1b64f1c1b0000001ull },
+                    { .ll = 0x1b90de3410000000ull },
+                    { .ll = 0x1b90de3410000000ull },
+                    { .ll = 0x5fcfffe4965a17e0ull },
+                    { .ll = 0x5fcfffe4965a17e0ull },
+                    { .ll = 0x2003ffffffffffffull },
+                    { .ll = 0x2003ffffffffffffull }
+                };
+
+    ll_fp a[] =
+                {
+                    { .ll = 0x00005ca8ull },
+                    { .ll = 0x0000badcull },
+                    { .ll = 0x7fdfffe816d77b00ull },
+                    { .d  = DBL_MAX }
+                };
+
+    ll_fp b[] =
+                {
+                    { .ll = 0x00001cefull },
+                    { .ll = 0x00005c70ull },
+                    { .ll = 0x7fdfffFC7F7FFF00ull },
+                    { .d  = 2.5 }
+                };
+
+    for (i = 0; i < 4; i++) {
+        t.d = asm_fmul(a[i].d, b[i].d);
+        if (t.ll != ch[2 * i].ll) {
+            ok = 0;
+            printf ("Mismatch on fmul n %d:\n\tresult:   %016llx\n\t"
+                    "expected: %016llx\n", i, t.ll, ch[2 * i].ll);
+        } else {
+            printf ("Ok on fmul n %d\n", i);
+        }
+        t.d = asm_fdiv(a[i].d, 1.0/b[i].d);
+        if (t.ll != ch[2 * i + 1].ll) {
+            ok = 0;
+            printf ("Mismatch on fdiv n %d:\n\tresult:   %016llx\n\t"
+                    "expected: %016llx\n", i, t.ll, ch[2 * i + 1].ll);
+        } else {
+            printf ("Ok on fdiv n %d\n", i);
+        }
+    }
+    fpscr.d = __builtin_mffs ();
+    printf("fpscr = %016llx\n", fpscr.ll);
+    if(!ok) {
+        return -1;
+    }
+    return 0;
+}
-- 
2.31.1

Re: [RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[Richard Henderson]

On 8/3/22 05:22, Lucas Mateus Castro(alqotel) wrote:
> From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>
> 
> As mentioned in the functions float_overflow_excp and
> float_underflow_excp, the result should be adjusted as mentioned in the
> ISA (subtracted 192/1536 from the exponent of the intermediate result if
> an overflow occurs with OE set and added 192/1536 to the exponent of the
> intermediate result if an underflow occurs with UE set), but at those
> functions the result has already been rounded so it is not possible to
> add/subtract from the intermediate result anymore.
>   
> This patch creates a new function that receives the value that should be
> subtracted/added from the exponent if an overflow/underflow happens, to
> not leave some arbitrary numbers from the PowerISA in the middle of the
> FPU code. If these numbers are 0 the new functions just call the old
> ones.
> 
> I used 2 values here for overflow and underflow, maybe it'd be better to
> just use the same ones, any thoughts?
> 
> Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
> ---
> An alternative I've thought was to always return the value adjusted if a
> overflow or underflow occurs and in float_underflow_excp and
> float_overflow_excp adjust it to inf/den/0 if OE/UE is 0, but I didn't
> saw many advantages to that approach.
> ---
>   fpu/softfloat.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>   include/fpu/softfloat.h |  2 ++
>   target/ppc/fpu_helper.c | 10 ++++--
>   3 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 4a871ef2a1..a407129dcb 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -268,6 +268,8 @@ typedef bool (*f64_check_fn)(union_float64 a, union_float64 b);
>   
>   typedef float32 (*soft_f32_op2_fn)(float32 a, float32 b, float_status *s);
>   typedef float64 (*soft_f64_op2_fn)(float64 a, float64 b, float_status *s);
> +typedef float64 (*soft_f64_op2_int2_fn)(float64 a, float64 b, int c, int d,
> +                                        float_status *s);
>   typedef float   (*hard_f32_op2_fn)(float a, float b);
>   typedef double  (*hard_f64_op2_fn)(double a, double b);
>   
> @@ -401,6 +403,19 @@ float64_gen2(float64 xa, float64 xb, float_status *s,
>       return soft(ua.s, ub.s, s);
>   }
>   
> +static inline float64
> +float64_gen2_excp(float64 xa, float64 xb, int xc, int xd, float_status *s,
> +                  hard_f64_op2_fn hard, soft_f64_op2_fn soft,
> +                  soft_f64_op2_int2_fn soft_excp, f64_check_fn pre,
> +                  f64_check_fn post)
> +{
> +    if (xc || xd) {
> +        return soft_excp(xa, xb, xc, xd, s);
> +    } else {
> +        return float64_gen2(xa, xb, s, hard, soft, pre, post);
> +    }
> +}
> +
>   /*
>    * Classify a floating point number. Everything above float_class_qnan
>    * is a NaN so cls >= float_class_qnan is any NaN.
> @@ -1929,6 +1944,39 @@ static double hard_f64_sub(double a, double b)
>       return a - b;
>   }
>   
> +static float64 QEMU_SOFTFLOAT_ATTR
> +soft_f64_addsub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
> +                        float_status *status, bool subtract)
> +{
> +    FloatParts64 pa, pb, *pr;
> +
> +    float64_unpack_canonical(&pa, a, status);
> +    float64_unpack_canonical(&pb, b, status);
> +    pr = parts_addsub(&pa, &pb, status, subtract);
> +
> +    if (unlikely(oe_sub && (pr->exp > 1023))) {
> +        pr->exp -= oe_sub;
> +        float_raise(float_flag_overflow, status);
> +    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
> +        pr->exp += ue_sum;
> +        float_raise(float_flag_underflow, status);
> +    }
> +
> +    return float64_round_pack_canonical(pr, status);

This is incorrect, because the exponent is not fixed until the middle of round_pack_canonical.

I think you should not add new functions like this, with new parameters, but instead add 
fields to float_status, which would then be checked at the places currently setting 
underflow and overflow.


r~

Re: [RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[Lucas Mateus Martins Araujo e Castro]

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


On 03/08/2022 13:18, Richard Henderson wrote:
>
> On 8/3/22 05:22, Lucas Mateus Castro(alqotel) wrote:
>> From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>
>>
>> As mentioned in the functions float_overflow_excp and
>> float_underflow_excp, the result should be adjusted as mentioned in the
>> ISA (subtracted 192/1536 from the exponent of the intermediate result if
>> an overflow occurs with OE set and added 192/1536 to the exponent of the
>> intermediate result if an underflow occurs with UE set), but at those
>> functions the result has already been rounded so it is not possible to
>> add/subtract from the intermediate result anymore.
>>
>> This patch creates a new function that receives the value that should be
>> subtracted/added from the exponent if an overflow/underflow happens, to
>> not leave some arbitrary numbers from the PowerISA in the middle of the
>> FPU code. If these numbers are 0 the new functions just call the old
>> ones.
>>
>> I used 2 values here for overflow and underflow, maybe it'd be better to
>> just use the same ones, any thoughts?
>>
>> Signed-off-by: Lucas Mateus Castro (alqotel) 
>> <lucas.araujo@eldorado.org.br>
>> ---
>> An alternative I've thought was to always return the value adjusted if a
>> overflow or underflow occurs and in float_underflow_excp and
>> float_overflow_excp adjust it to inf/den/0 if OE/UE is 0, but I didn't
>> saw many advantages to that approach.
>> ---
>>   fpu/softfloat.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>>   include/fpu/softfloat.h |  2 ++
>>   target/ppc/fpu_helper.c | 10 ++++--
>>   3 files changed, 85 insertions(+), 2 deletions(-)
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index 4a871ef2a1..a407129dcb 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -268,6 +268,8 @@ typedef bool (*f64_check_fn)(union_float64 a, 
>> union_float64 b);
>>
>>   typedef float32 (*soft_f32_op2_fn)(float32 a, float32 b, 
>> float_status *s);
>>   typedef float64 (*soft_f64_op2_fn)(float64 a, float64 b, 
>> float_status *s);
>> +typedef float64 (*soft_f64_op2_int2_fn)(float64 a, float64 b, int c, 
>> int d,
>> +                                        float_status *s);
>>   typedef float   (*hard_f32_op2_fn)(float a, float b);
>>   typedef double  (*hard_f64_op2_fn)(double a, double b);
>>
>> @@ -401,6 +403,19 @@ float64_gen2(float64 xa, float64 xb, 
>> float_status *s,
>>       return soft(ua.s, ub.s, s);
>>   }
>>
>> +static inline float64
>> +float64_gen2_excp(float64 xa, float64 xb, int xc, int xd, 
>> float_status *s,
>> +                  hard_f64_op2_fn hard, soft_f64_op2_fn soft,
>> +                  soft_f64_op2_int2_fn soft_excp, f64_check_fn pre,
>> +                  f64_check_fn post)
>> +{
>> +    if (xc || xd) {
>> +        return soft_excp(xa, xb, xc, xd, s);
>> +    } else {
>> +        return float64_gen2(xa, xb, s, hard, soft, pre, post);
>> +    }
>> +}
>> +
>>   /*
>>    * Classify a floating point number. Everything above float_class_qnan
>>    * is a NaN so cls >= float_class_qnan is any NaN.
>> @@ -1929,6 +1944,39 @@ static double hard_f64_sub(double a, double b)
>>       return a - b;
>>   }
>>
>> +static float64 QEMU_SOFTFLOAT_ATTR
>> +soft_f64_addsub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
>> +                        float_status *status, bool subtract)
>> +{
>> +    FloatParts64 pa, pb, *pr;
>> +
>> +    float64_unpack_canonical(&pa, a, status);
>> +    float64_unpack_canonical(&pb, b, status);
>> +    pr = parts_addsub(&pa, &pb, status, subtract);
>> +
>> +    if (unlikely(oe_sub && (pr->exp > 1023))) {
>> +        pr->exp -= oe_sub;
>> +        float_raise(float_flag_overflow, status);
>> +    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
>> +        pr->exp += ue_sum;
>> +        float_raise(float_flag_underflow, status);
>> +    }
>> +
>> +    return float64_round_pack_canonical(pr, status);
>
> This is incorrect, because the exponent is not fixed until the middle 
> of round_pack_canonical.
>
> I think you should not add new functions like this, with new 
> parameters, but instead add
> fields to float_status, which would then be checked at the places 
> currently setting
> underflow and overflow.

So add overflow_correction and underflow_correction in 
'partsN(uncanon_normal)' so that:

if (exp >= exp_max) {
     if (overflow_correction != 0) {
         exp -= overflow_correction;
     }
}

And the equivalent for underflow, or a bool ppc_overflow_enable that 
uses a fixed value like:

if (exp >= exp_max) {
     if (ppc_overflow_enable) {
         exp -= ((fmt->exp_bias + 1) + (fmt->exp_bias + 1)/2);
     }
}

(and the equivalent for underflow) ?

>
>
> r~
-- 
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 8301 bytes --]

Re: [RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[Richard Henderson]

On 8/3/22 10:45, Lucas Mateus Martins Araujo e Castro wrote:
> 
> On 03/08/2022 13:18, Richard Henderson wrote:
>>
>> On 8/3/22 05:22, Lucas Mateus Castro(alqotel) wrote:
>>> From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>
>>>
>>> As mentioned in the functions float_overflow_excp and
>>> float_underflow_excp, the result should be adjusted as mentioned in the
>>> ISA (subtracted 192/1536 from the exponent of the intermediate result if
>>> an overflow occurs with OE set and added 192/1536 to the exponent of the
>>> intermediate result if an underflow occurs with UE set), but at those
>>> functions the result has already been rounded so it is not possible to
>>> add/subtract from the intermediate result anymore.
>>>
>>> This patch creates a new function that receives the value that should be
>>> subtracted/added from the exponent if an overflow/underflow happens, to
>>> not leave some arbitrary numbers from the PowerISA in the middle of the
>>> FPU code. If these numbers are 0 the new functions just call the old
>>> ones.
>>>
>>> I used 2 values here for overflow and underflow, maybe it'd be better to
>>> just use the same ones, any thoughts?
>>>
>>> Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br>
>>> ---
>>> An alternative I've thought was to always return the value adjusted if a
>>> overflow or underflow occurs and in float_underflow_excp and
>>> float_overflow_excp adjust it to inf/den/0 if OE/UE is 0, but I didn't
>>> saw many advantages to that approach.
>>> ---
>>>   fpu/softfloat.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>>>   include/fpu/softfloat.h |  2 ++
>>>   target/ppc/fpu_helper.c | 10 ++++--
>>>   3 files changed, 85 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>>> index 4a871ef2a1..a407129dcb 100644
>>> --- a/fpu/softfloat.c
>>> +++ b/fpu/softfloat.c
>>> @@ -268,6 +268,8 @@ typedef bool (*f64_check_fn)(union_float64 a, union_float64 b);
>>>
>>>   typedef float32 (*soft_f32_op2_fn)(float32 a, float32 b, float_status *s);
>>>   typedef float64 (*soft_f64_op2_fn)(float64 a, float64 b, float_status *s);
>>> +typedef float64 (*soft_f64_op2_int2_fn)(float64 a, float64 b, int c, int d,
>>> +                                        float_status *s);
>>>   typedef float   (*hard_f32_op2_fn)(float a, float b);
>>>   typedef double  (*hard_f64_op2_fn)(double a, double b);
>>>
>>> @@ -401,6 +403,19 @@ float64_gen2(float64 xa, float64 xb, float_status *s,
>>>       return soft(ua.s, ub.s, s);
>>>   }
>>>
>>> +static inline float64
>>> +float64_gen2_excp(float64 xa, float64 xb, int xc, int xd, float_status *s,
>>> +                  hard_f64_op2_fn hard, soft_f64_op2_fn soft,
>>> +                  soft_f64_op2_int2_fn soft_excp, f64_check_fn pre,
>>> +                  f64_check_fn post)
>>> +{
>>> +    if (xc || xd) {
>>> +        return soft_excp(xa, xb, xc, xd, s);
>>> +    } else {
>>> +        return float64_gen2(xa, xb, s, hard, soft, pre, post);
>>> +    }
>>> +}
>>> +
>>>   /*
>>>    * Classify a floating point number. Everything above float_class_qnan
>>>    * is a NaN so cls >= float_class_qnan is any NaN.
>>> @@ -1929,6 +1944,39 @@ static double hard_f64_sub(double a, double b)
>>>       return a - b;
>>>   }
>>>
>>> +static float64 QEMU_SOFTFLOAT_ATTR
>>> +soft_f64_addsub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
>>> +                        float_status *status, bool subtract)
>>> +{
>>> +    FloatParts64 pa, pb, *pr;
>>> +
>>> +    float64_unpack_canonical(&pa, a, status);
>>> +    float64_unpack_canonical(&pb, b, status);
>>> +    pr = parts_addsub(&pa, &pb, status, subtract);
>>> +
>>> +    if (unlikely(oe_sub && (pr->exp > 1023))) {
>>> +        pr->exp -= oe_sub;
>>> +        float_raise(float_flag_overflow, status);
>>> +    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
>>> +        pr->exp += ue_sum;
>>> +        float_raise(float_flag_underflow, status);
>>> +    }
>>> +
>>> +    return float64_round_pack_canonical(pr, status);
>>
>> This is incorrect, because the exponent is not fixed until the middle of 
>> round_pack_canonical.
>>
>> I think you should not add new functions like this, with new parameters, but instead add
>> fields to float_status, which would then be checked at the places currently setting
>> underflow and overflow.
> 
> So add overflow_correction and underflow_correction in 'partsN(uncanon_normal)' so that:
> 
> if (exp >= exp_max) {
>      if (overflow_correction != 0) {
>          exp -= overflow_correction;
>      }
> }
> 
> And the equivalent for underflow, or a bool ppc_overflow_enable that uses a fixed value like:
> 
> if (exp >= exp_max) {
>      if (ppc_overflow_enable) {
>          exp -= ((fmt->exp_bias + 1) + (fmt->exp_bias + 1)/2);
>      }
> }
> 
> (and the equivalent for underflow) ?

Something like that.

I would suggest pre-computing that adjustment into fmt, via FLOAT_PARAMS.
Naming is always hard, but how about exp_re_bias?

The flag(s) should not contain "ppc" in the name.  But perhaps

   s->rebias_overflow
   s->rebias_underflow


r~

Re: [RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[Lucas Mateus Martins Araujo e Castro]

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


On 03/08/2022 15:16, Richard Henderson wrote:
>
> On 8/3/22 10:45, Lucas Mateus Martins Araujo e Castro wrote:
>>
>> On 03/08/2022 13:18, Richard Henderson wrote:
>>>
>>> On 8/3/22 05:22, Lucas Mateus Castro(alqotel) wrote:
>>>> From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>
>>>>
>>>> As mentioned in the functions float_overflow_excp and
>>>> float_underflow_excp, the result should be adjusted as mentioned in 
>>>> the
>>>> ISA (subtracted 192/1536 from the exponent of the intermediate 
>>>> result if
>>>> an overflow occurs with OE set and added 192/1536 to the exponent 
>>>> of the
>>>> intermediate result if an underflow occurs with UE set), but at those
>>>> functions the result has already been rounded so it is not possible to
>>>> add/subtract from the intermediate result anymore.
>>>>
>>>> This patch creates a new function that receives the value that 
>>>> should be
>>>> subtracted/added from the exponent if an overflow/underflow 
>>>> happens, to
>>>> not leave some arbitrary numbers from the PowerISA in the middle of 
>>>> the
>>>> FPU code. If these numbers are 0 the new functions just call the old
>>>> ones.
>>>>
>>>> I used 2 values here for overflow and underflow, maybe it'd be 
>>>> better to
>>>> just use the same ones, any thoughts?
>>>>
>>>> Signed-off-by: Lucas Mateus Castro (alqotel) 
>>>> <lucas.araujo@eldorado.org.br>
>>>> ---
>>>> An alternative I've thought was to always return the value adjusted 
>>>> if a
>>>> overflow or underflow occurs and in float_underflow_excp and
>>>> float_overflow_excp adjust it to inf/den/0 if OE/UE is 0, but I didn't
>>>> saw many advantages to that approach.
>>>> ---
>>>>   fpu/softfloat.c         | 75 
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>   include/fpu/softfloat.h |  2 ++
>>>>   target/ppc/fpu_helper.c | 10 ++++--
>>>>   3 files changed, 85 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>>>> index 4a871ef2a1..a407129dcb 100644
>>>> --- a/fpu/softfloat.c
>>>> +++ b/fpu/softfloat.c
>>>> @@ -268,6 +268,8 @@ typedef bool (*f64_check_fn)(union_float64 a, 
>>>> union_float64 b);
>>>>
>>>>   typedef float32 (*soft_f32_op2_fn)(float32 a, float32 b, 
>>>> float_status *s);
>>>>   typedef float64 (*soft_f64_op2_fn)(float64 a, float64 b, 
>>>> float_status *s);
>>>> +typedef float64 (*soft_f64_op2_int2_fn)(float64 a, float64 b, int 
>>>> c, int d,
>>>> +                                        float_status *s);
>>>>   typedef float   (*hard_f32_op2_fn)(float a, float b);
>>>>   typedef double  (*hard_f64_op2_fn)(double a, double b);
>>>>
>>>> @@ -401,6 +403,19 @@ float64_gen2(float64 xa, float64 xb, 
>>>> float_status *s,
>>>>       return soft(ua.s, ub.s, s);
>>>>   }
>>>>
>>>> +static inline float64
>>>> +float64_gen2_excp(float64 xa, float64 xb, int xc, int xd, 
>>>> float_status *s,
>>>> +                  hard_f64_op2_fn hard, soft_f64_op2_fn soft,
>>>> +                  soft_f64_op2_int2_fn soft_excp, f64_check_fn pre,
>>>> +                  f64_check_fn post)
>>>> +{
>>>> +    if (xc || xd) {
>>>> +        return soft_excp(xa, xb, xc, xd, s);
>>>> +    } else {
>>>> +        return float64_gen2(xa, xb, s, hard, soft, pre, post);
>>>> +    }
>>>> +}
>>>> +
>>>>   /*
>>>>    * Classify a floating point number. Everything above 
>>>> float_class_qnan
>>>>    * is a NaN so cls >= float_class_qnan is any NaN.
>>>> @@ -1929,6 +1944,39 @@ static double hard_f64_sub(double a, double b)
>>>>       return a - b;
>>>>   }
>>>>
>>>> +static float64 QEMU_SOFTFLOAT_ATTR
>>>> +soft_f64_addsub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
>>>> +                        float_status *status, bool subtract)
>>>> +{
>>>> +    FloatParts64 pa, pb, *pr;
>>>> +
>>>> +    float64_unpack_canonical(&pa, a, status);
>>>> +    float64_unpack_canonical(&pb, b, status);
>>>> +    pr = parts_addsub(&pa, &pb, status, subtract);
>>>> +
>>>> +    if (unlikely(oe_sub && (pr->exp > 1023))) {
>>>> +        pr->exp -= oe_sub;
>>>> +        float_raise(float_flag_overflow, status);
>>>> +    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
>>>> +        pr->exp += ue_sum;
>>>> +        float_raise(float_flag_underflow, status);
>>>> +    }
>>>> +
>>>> +    return float64_round_pack_canonical(pr, status);
>>>
>>> This is incorrect, because the exponent is not fixed until the 
>>> middle of
>>> round_pack_canonical.
>>>
>>> I think you should not add new functions like this, with new 
>>> parameters, but instead add
>>> fields to float_status, which would then be checked at the places 
>>> currently setting
>>> underflow and overflow.
>>
>> So add overflow_correction and underflow_correction in 
>> 'partsN(uncanon_normal)' so that:
>>
>> if (exp >= exp_max) {
>>      if (overflow_correction != 0) {
>>          exp -= overflow_correction;
>>      }
>> }
>>
>> And the equivalent for underflow, or a bool ppc_overflow_enable that 
>> uses a fixed value like:
>>
>> if (exp >= exp_max) {
>>      if (ppc_overflow_enable) {
>>          exp -= ((fmt->exp_bias + 1) + (fmt->exp_bias + 1)/2);
>>      }
>> }
>>
>> (and the equivalent for underflow) ?
>
> Something like that.
>
> I would suggest pre-computing that adjustment into fmt, via FLOAT_PARAMS.
> Naming is always hard, but how about exp_re_bias?
>
> The flag(s) should not contain "ppc" in the name.  But perhaps
>
>   s->rebias_overflow
>   s->rebias_underflow

rebias_* sounds good to me.

Also I imagine that these bools would be set by mtfsf, mtfsfi, mtfsb0 
and mtfsb1, in which case it'd make these patches significantly shorter. 
I'll send a v2 with these changes

>
>
>
> r~
-- 
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 10408 bytes --]