[PATCH for-10.0 00/11] fpu: pickNaN follow ups

[PATCH for-10.0 00/11] fpu: pickNaN follow ups

[Richard Henderson]

Based-on: 20241202131347.498124-55-peter.maydell@linaro.org
([PATCH v2 for-10.0 54/54] fpu: Remove default handling for dnan_pattern)

The first patch needs to be inserted into Peter's patch set,
any place before 54/54 when dnan_pattern becomes mandatory.

The rest are cleanups that become possible when the
softfloat-specialize.c.inc functions are inlined into their
single callers in softfloat-parts.c.inc.


r~


Richard Henderson (11):
  target/arm: Copy entire float_status in is_ebf
  softfloat: Inline pickNaNMulAdd
  softfloat: Use goto for default nan case in pick_nan_muladd
  softfloat: Remove which from parts_pick_nan_muladd
  softfloat: Pad array size in pick_nan_muladd
  softfloat: Move propagateFloatx80NaN to softfloat.c
  softfloat: Use parts_pick_nan in propagateFloatx80NaN
  softfloat: Inline pickNaN
  softfloat: Share code between parts_pick_nan cases
  softfloat: Sink frac_cmp in parts_pick_nan until needed
  softfloat: Replace WHICH with RET in parts_pick_nan

 fpu/softfloat.c                |  19 ++++
 target/arm/tcg/vec_helper.c    |  20 ++--
 fpu/softfloat-parts.c.inc      | 136 +++++++++++++++++-----
 fpu/softfloat-specialize.c.inc | 202 ---------------------------------
 4 files changed, 131 insertions(+), 246 deletions(-)

-- 
2.43.0

[PATCH 01/11] target/arm: Copy entire float_status in is_ebf

[Richard Henderson]

Now that float_status has a bunch of fp parameters,
it is easier to copy an existing structure than create
one from scratch.  Begin by copying the structure that
corresponds to the FPSR and make only the adjustments
required for BFloat16 semantics.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/tcg/vec_helper.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/target/arm/tcg/vec_helper.c b/target/arm/tcg/vec_helper.c
index e825d501a2..ad6f26545a 100644
--- a/target/arm/tcg/vec_helper.c
+++ b/target/arm/tcg/vec_helper.c
@@ -2813,25 +2813,19 @@ bool is_ebf(CPUARMState *env, float_status *statusp, float_status *oddstatusp)
      * no effect on AArch32 instructions.
      */
     bool ebf = is_a64(env) && env->vfp.fpcr & FPCR_EBF;
-    *statusp = (float_status){
-        .tininess_before_rounding = float_tininess_before_rounding,
-        .float_rounding_mode = float_round_to_odd_inf,
-        .flush_to_zero = true,
-        .flush_inputs_to_zero = true,
-        .default_nan_mode = true,
-    };
+
+    *statusp = env->vfp.fp_status;
+    set_default_nan_mode(true, statusp);
 
     if (ebf) {
-        float_status *fpst = &env->vfp.fp_status;
-        set_flush_to_zero(get_flush_to_zero(fpst), statusp);
-        set_flush_inputs_to_zero(get_flush_inputs_to_zero(fpst), statusp);
-        set_float_rounding_mode(get_float_rounding_mode(fpst), statusp);
-
         /* EBF=1 needs to do a step with round-to-odd semantics */
         *oddstatusp = *statusp;
         set_float_rounding_mode(float_round_to_odd, oddstatusp);
+    } else {
+        set_flush_to_zero(true, statusp);
+        set_flush_inputs_to_zero(true, statusp);
+        set_float_rounding_mode(float_round_to_odd_inf, statusp);
     }
-
     return ebf;
 }
 
-- 
2.43.0

Re: [PATCH 01/11] target/arm: Copy entire float_status in is_ebf

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> Now that float_status has a bunch of fp parameters,
> it is easier to copy an existing structure than create
> one from scratch.  Begin by copying the structure that
> corresponds to the FPSR and make only the adjustments
> required for BFloat16 semantics.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/tcg/vec_helper.c | 20 +++++++-------------
>   1 file changed, 7 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/11] target/arm: Copy entire float_status in is_ebf

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Now that float_status has a bunch of fp parameters,
> it is easier to copy an existing structure than create
> one from scratch.  Begin by copying the structure that
> corresponds to the FPSR and make only the adjustments
> required for BFloat16 semantics.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/tcg/vec_helper.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I missed this in my grep for places where we set up
a float_status I think because it's the only place where
we use a struct initializer in a way that isn't literally
 "float_status s = { ... }". But I did a wider grep and
I think this is the only place I missed like that.

thanks
-- PMM

[PATCH 02/11] softfloat: Inline pickNaNMulAdd

[Richard Henderson]

Inline pickNaNMulAdd into its only caller.  This makes
one assert redundant with the immediately preceding IF.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc      | 35 +++++++++++++++++++++-
 fpu/softfloat-specialize.c.inc | 54 ----------------------------------
 2 files changed, 34 insertions(+), 55 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 655b7d9da5..f5c6b21fee 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -80,8 +80,41 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
 
     if (s->default_nan_mode) {
         which = 3;
+    } else if (infzero) {
+        /*
+         * Inf * 0 + NaN -- some implementations return the
+         * default NaN here, and some return the input NaN.
+         */
+        switch (s->float_infzeronan_rule) {
+        case float_infzeronan_dnan_never:
+            which = 2;
+            break;
+        case float_infzeronan_dnan_always:
+            which = 3;
+            break;
+        case float_infzeronan_dnan_if_qnan:
+            which = is_qnan(c->cls) ? 3 : 2;
+            break;
+        default:
+            g_assert_not_reached();
+        }
     } else {
-        which = pickNaNMulAdd(a->cls, b->cls, c->cls, infzero, have_snan, s);
+        FloatClass cls[3] = { a->cls, b->cls, c->cls };
+        Float3NaNPropRule rule = s->float_3nan_prop_rule;
+
+        assert(rule != float_3nan_prop_none);
+        if (have_snan && (rule & R_3NAN_SNAN_MASK)) {
+            /* We have at least one SNaN input and should prefer it */
+            do {
+                which = rule & R_3NAN_1ST_MASK;
+                rule >>= R_3NAN_1ST_LENGTH;
+            } while (!is_snan(cls[which]));
+        } else {
+            do {
+                which = rule & R_3NAN_1ST_MASK;
+                rule >>= R_3NAN_1ST_LENGTH;
+            } while (!is_nan(cls[which]));
+        }
     }
 
     if (which == 3) {
diff --git a/fpu/softfloat-specialize.c.inc b/fpu/softfloat-specialize.c.inc
index e075c47889..f26458eaa3 100644
--- a/fpu/softfloat-specialize.c.inc
+++ b/fpu/softfloat-specialize.c.inc
@@ -448,60 +448,6 @@ static int pickNaN(FloatClass a_cls, FloatClass b_cls,
     }
 }
 
-/*----------------------------------------------------------------------------
-| Select which NaN to propagate for a three-input operation.
-| For the moment we assume that no CPU needs the 'larger significand'
-| information.
-| Return values : 0 : a; 1 : b; 2 : c; 3 : default-NaN
-*----------------------------------------------------------------------------*/
-static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
-                         bool infzero, bool have_snan, float_status *status)
-{
-    FloatClass cls[3] = { a_cls, b_cls, c_cls };
-    Float3NaNPropRule rule = status->float_3nan_prop_rule;
-    int which;
-
-    /*
-     * We guarantee not to require the target to tell us how to
-     * pick a NaN if we're always returning the default NaN.
-     * But if we're not in default-NaN mode then the target must
-     * specify.
-     */
-    assert(!status->default_nan_mode);
-
-    if (infzero) {
-        /*
-         * Inf * 0 + NaN -- some implementations return the default NaN here,
-         * and some return the input NaN.
-         */
-        switch (status->float_infzeronan_rule) {
-        case float_infzeronan_dnan_never:
-            return 2;
-        case float_infzeronan_dnan_always:
-            return 3;
-        case float_infzeronan_dnan_if_qnan:
-            return is_qnan(c_cls) ? 3 : 2;
-        default:
-            g_assert_not_reached();
-        }
-    }
-
-    assert(rule != float_3nan_prop_none);
-    if (have_snan && (rule & R_3NAN_SNAN_MASK)) {
-        /* We have at least one SNaN input and should prefer it */
-        do {
-            which = rule & R_3NAN_1ST_MASK;
-            rule >>= R_3NAN_1ST_LENGTH;
-        } while (!is_snan(cls[which]));
-    } else {
-        do {
-            which = rule & R_3NAN_1ST_MASK;
-            rule >>= R_3NAN_1ST_LENGTH;
-        } while (!is_nan(cls[which]));
-    }
-    return which;
-}
-
 /*----------------------------------------------------------------------------
 | Returns 1 if the double-precision floating-point value `a' is a quiet
 | NaN; otherwise returns 0.
-- 
2.43.0

Re: [PATCH 02/11] softfloat: Inline pickNaNMulAdd

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> Inline pickNaNMulAdd into its only caller.  This makes
> one assert redundant with the immediately preceding IF.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   fpu/softfloat-parts.c.inc      | 35 +++++++++++++++++++++-
>   fpu/softfloat-specialize.c.inc | 54 ----------------------------------
>   2 files changed, 34 insertions(+), 55 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 02/11] softfloat: Inline pickNaNMulAdd

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Inline pickNaNMulAdd into its only caller.  This makes
> one assert redundant with the immediately preceding IF.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  fpu/softfloat-parts.c.inc      | 35 +++++++++++++++++++++-
>  fpu/softfloat-specialize.c.inc | 54 ----------------------------------
>  2 files changed, 34 insertions(+), 55 deletions(-)

> -/*----------------------------------------------------------------------------
> -| Select which NaN to propagate for a three-input operation.
> -| For the moment we assume that no CPU needs the 'larger significand'
> -| information.
> -| Return values : 0 : a; 1 : b; 2 : c; 3 : default-NaN
> -*----------------------------------------------------------------------------*/
> -static int pickNaNMulAdd(FloatClass a_cls, FloatClass b_cls, FloatClass c_cls,
> -                         bool infzero, bool have_snan, float_status *status)
> -{
> -    FloatClass cls[3] = { a_cls, b_cls, c_cls };
> -    Float3NaNPropRule rule = status->float_3nan_prop_rule;
> -    int which;
> -
> -    /*
> -     * We guarantee not to require the target to tell us how to
> -     * pick a NaN if we're always returning the default NaN.
> -     * But if we're not in default-NaN mode then the target must
> -     * specify.
> -     */

This comment has got lost in the refactoring. Since it's
documenting a guarantee we make to the target, I think
it's worth retaining.

thanks
-- PMM

[PATCH 03/11] softfloat: Use goto for default nan case in pick_nan_muladd

[Richard Henderson]

Remove "3" as a special case for which and simply
branch to return the desired value.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index f5c6b21fee..6423e12406 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -79,7 +79,7 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
     }
 
     if (s->default_nan_mode) {
-        which = 3;
+        goto default_nan;
     } else if (infzero) {
         /*
          * Inf * 0 + NaN -- some implementations return the
@@ -87,17 +87,18 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
          */
         switch (s->float_infzeronan_rule) {
         case float_infzeronan_dnan_never:
-            which = 2;
             break;
         case float_infzeronan_dnan_always:
-            which = 3;
-            break;
+            goto default_nan;
         case float_infzeronan_dnan_if_qnan:
-            which = is_qnan(c->cls) ? 3 : 2;
+            if (is_qnan(c->cls)) {
+                goto default_nan;
+            }
             break;
         default:
             g_assert_not_reached();
         }
+        which = 2;
     } else {
         FloatClass cls[3] = { a->cls, b->cls, c->cls };
         Float3NaNPropRule rule = s->float_3nan_prop_rule;
@@ -117,11 +118,6 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
         }
     }
 
-    if (which == 3) {
-        parts_default_nan(a, s);
-        return a;
-    }
-
     switch (which) {
     case 0:
         break;
@@ -138,6 +134,10 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
         parts_silence_nan(a, s);
     }
     return a;
+
+ default_nan:
+    parts_default_nan(a, s);
+    return a;
 }
 
 /*
-- 
2.43.0

Re: [PATCH 03/11] softfloat: Use goto for default nan case in pick_nan_muladd

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> Remove "3" as a special case for which and simply
> branch to return the desired value.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   fpu/softfloat-parts.c.inc | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 04/11] softfloat: Remove which from parts_pick_nan_muladd

[Richard Henderson]

Assign the pointer return value to 'a' directly,
rather than going through an intermediary index.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 6423e12406..77f16ac158 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -65,9 +65,9 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
                                             FloatPartsN *c, float_status *s,
                                             int ab_mask, int abc_mask)
 {
-    int which;
     bool infzero = (ab_mask == float_cmask_infzero);
     bool have_snan = (abc_mask & float_cmask_snan);
+    FloatPartsN *ret;
 
     if (unlikely(have_snan)) {
         float_raise(float_flag_invalid | float_flag_invalid_snan, s);
@@ -98,42 +98,30 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
         default:
             g_assert_not_reached();
         }
-        which = 2;
+        ret = c;
     } else {
-        FloatClass cls[3] = { a->cls, b->cls, c->cls };
+        FloatPartsN *val[3] = { a, b, c };
         Float3NaNPropRule rule = s->float_3nan_prop_rule;
 
         assert(rule != float_3nan_prop_none);
         if (have_snan && (rule & R_3NAN_SNAN_MASK)) {
             /* We have at least one SNaN input and should prefer it */
             do {
-                which = rule & R_3NAN_1ST_MASK;
+                ret = val[rule & R_3NAN_1ST_MASK];
                 rule >>= R_3NAN_1ST_LENGTH;
-            } while (!is_snan(cls[which]));
+            } while (!is_snan(ret->cls));
         } else {
             do {
-                which = rule & R_3NAN_1ST_MASK;
+                ret = val[rule & R_3NAN_1ST_MASK];
                 rule >>= R_3NAN_1ST_LENGTH;
-            } while (!is_nan(cls[which]));
+            } while (!is_nan(ret->cls));
         }
     }
 
-    switch (which) {
-    case 0:
-        break;
-    case 1:
-        a = b;
-        break;
-    case 2:
-        a = c;
-        break;
-    default:
-        g_assert_not_reached();
+    if (is_snan(ret->cls)) {
+        parts_silence_nan(ret, s);
     }
-    if (is_snan(a->cls)) {
-        parts_silence_nan(a, s);
-    }
-    return a;
+    return ret;
 
  default_nan:
     parts_default_nan(a, s);
-- 
2.43.0

Re: [PATCH 04/11] softfloat: Remove which from parts_pick_nan_muladd

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> Assign the pointer return value to 'a' directly,
> rather than going through an intermediary index.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   fpu/softfloat-parts.c.inc | 32 ++++++++++----------------------
>   1 file changed, 10 insertions(+), 22 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 05/11] softfloat: Pad array size in pick_nan_muladd

[Richard Henderson]

While all indices into val[] should be in [0-2], the mask
applied is two bits.  To help static analysis see there is
no possibility of read beyond the end of the array, pad the
array to 4 entries, with the final being (implicitly) NULL.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 77f16ac158..06cfc6abb5 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -100,7 +100,7 @@ static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
         }
         ret = c;
     } else {
-        FloatPartsN *val[3] = { a, b, c };
+        FloatPartsN *val[R_3NAN_1ST_MASK + 1] = { a, b, c };
         Float3NaNPropRule rule = s->float_3nan_prop_rule;
 
         assert(rule != float_3nan_prop_none);
-- 
2.43.0

Re: [PATCH 05/11] softfloat: Pad array size in pick_nan_muladd

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> While all indices into val[] should be in [0-2], the mask
> applied is two bits.  To help static analysis see there is
> no possibility of read beyond the end of the array, pad the
> array to 4 entries, with the final being (implicitly) NULL.

Squash in previous keeping the explanation? Regardless,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   fpu/softfloat-parts.c.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH 06/11] softfloat: Move propagateFloatx80NaN to softfloat.c

[Richard Henderson]

This function is part of the public interface and
is not "specialized" to any target in any way.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat.c                | 52 ++++++++++++++++++++++++++++++++++
 fpu/softfloat-specialize.c.inc | 52 ----------------------------------
 2 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 027a8e576d..6ba1cfd32a 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -4920,6 +4920,58 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
     *zExpPtr = 1 - shiftCount;
 }
 
+/*----------------------------------------------------------------------------
+| Takes two extended double-precision floating-point values `a' and `b', one
+| of which is a NaN, and returns the appropriate NaN result.  If either `a' or
+| `b' is a signaling NaN, the invalid exception is raised.
+*----------------------------------------------------------------------------*/
+
+floatx80 propagateFloatx80NaN(floatx80 a, floatx80 b, float_status *status)
+{
+    bool aIsLargerSignificand;
+    FloatClass a_cls, b_cls;
+
+    /* This is not complete, but is good enough for pickNaN.  */
+    a_cls = (!floatx80_is_any_nan(a)
+             ? float_class_normal
+             : floatx80_is_signaling_nan(a, status)
+             ? float_class_snan
+             : float_class_qnan);
+    b_cls = (!floatx80_is_any_nan(b)
+             ? float_class_normal
+             : floatx80_is_signaling_nan(b, status)
+             ? float_class_snan
+             : float_class_qnan);
+
+    if (is_snan(a_cls) || is_snan(b_cls)) {
+        float_raise(float_flag_invalid, status);
+    }
+
+    if (status->default_nan_mode) {
+        return floatx80_default_nan(status);
+    }
+
+    if (a.low < b.low) {
+        aIsLargerSignificand = 0;
+    } else if (b.low < a.low) {
+        aIsLargerSignificand = 1;
+    } else {
+        aIsLargerSignificand = (a.high < b.high) ? 1 : 0;
+    }
+
+    if (pickNaN(a_cls, b_cls, aIsLargerSignificand, status)) {
+        if (is_snan(b_cls)) {
+            return floatx80_silence_nan(b, status);
+        }
+        return b;
+    } else {
+        if (is_snan(a_cls)) {
+            return floatx80_silence_nan(a, status);
+        }
+        return a;
+    }
+}
+
 /*----------------------------------------------------------------------------
 | Takes an abstract floating-point value having sign `zSign', exponent `zExp',
 | and extended significand formed by the concatenation of `zSig0' and `zSig1',
diff --git a/fpu/softfloat-specialize.c.inc b/fpu/softfloat-specialize.c.inc
index f26458eaa3..f7a320f6ff 100644
--- a/fpu/softfloat-specialize.c.inc
+++ b/fpu/softfloat-specialize.c.inc
@@ -551,58 +551,6 @@ floatx80 floatx80_silence_nan(floatx80 a, float_status *status)
     return a;
 }
 
-/*----------------------------------------------------------------------------
-| Takes two extended double-precision floating-point values `a' and `b', one
-| of which is a NaN, and returns the appropriate NaN result.  If either `a' or
-| `b' is a signaling NaN, the invalid exception is raised.
-*----------------------------------------------------------------------------*/
-
-floatx80 propagateFloatx80NaN(floatx80 a, floatx80 b, float_status *status)
-{
-    bool aIsLargerSignificand;
-    FloatClass a_cls, b_cls;
-
-    /* This is not complete, but is good enough for pickNaN.  */
-    a_cls = (!floatx80_is_any_nan(a)
-             ? float_class_normal
-             : floatx80_is_signaling_nan(a, status)
-             ? float_class_snan
-             : float_class_qnan);
-    b_cls = (!floatx80_is_any_nan(b)
-             ? float_class_normal
-             : floatx80_is_signaling_nan(b, status)
-             ? float_class_snan
-             : float_class_qnan);
-
-    if (is_snan(a_cls) || is_snan(b_cls)) {
-        float_raise(float_flag_invalid, status);
-    }
-
-    if (status->default_nan_mode) {
-        return floatx80_default_nan(status);
-    }
-
-    if (a.low < b.low) {
-        aIsLargerSignificand = 0;
-    } else if (b.low < a.low) {
-        aIsLargerSignificand = 1;
-    } else {
-        aIsLargerSignificand = (a.high < b.high) ? 1 : 0;
-    }
-
-    if (pickNaN(a_cls, b_cls, aIsLargerSignificand, status)) {
-        if (is_snan(b_cls)) {
-            return floatx80_silence_nan(b, status);
-        }
-        return b;
-    } else {
-        if (is_snan(a_cls)) {
-            return floatx80_silence_nan(a, status);
-        }
-        return a;
-    }
-}
-
 /*----------------------------------------------------------------------------
 | Returns 1 if the quadruple-precision floating-point value `a' is a quiet
 | NaN; otherwise returns 0.
-- 
2.43.0

Re: [PATCH 06/11] softfloat: Move propagateFloatx80NaN to softfloat.c

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This function is part of the public interface and
> is not "specialized" to any target in any way.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 07/11] softfloat: Use parts_pick_nan in propagateFloatx80NaN

[Richard Henderson]

Unpacking and repacking the parts may be slightly more work
than we did before, but we get to reuse more code.  For a
code path handling exceptional values, this is an improvement.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat.c | 43 +++++--------------------------------------
 1 file changed, 5 insertions(+), 38 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 6ba1cfd32a..8de8d5f342 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -4928,48 +4928,15 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
 
 floatx80 propagateFloatx80NaN(floatx80 a, floatx80 b, float_status *status)
 {
-    bool aIsLargerSignificand;
-    FloatClass a_cls, b_cls;
+    FloatParts128 pa, pb, *pr;
 
-    /* This is not complete, but is good enough for pickNaN.  */
-    a_cls = (!floatx80_is_any_nan(a)
-             ? float_class_normal
-             : floatx80_is_signaling_nan(a, status)
-             ? float_class_snan
-             : float_class_qnan);
-    b_cls = (!floatx80_is_any_nan(b)
-             ? float_class_normal
-             : floatx80_is_signaling_nan(b, status)
-             ? float_class_snan
-             : float_class_qnan);
-
-    if (is_snan(a_cls) || is_snan(b_cls)) {
-        float_raise(float_flag_invalid, status);
-    }
-
-    if (status->default_nan_mode) {
+    if (!floatx80_unpack_canonical(&pa, a, status) ||
+        !floatx80_unpack_canonical(&pb, b, status)) {
         return floatx80_default_nan(status);
     }
 
-    if (a.low < b.low) {
-        aIsLargerSignificand = 0;
-    } else if (b.low < a.low) {
-        aIsLargerSignificand = 1;
-    } else {
-        aIsLargerSignificand = (a.high < b.high) ? 1 : 0;
-    }
-
-    if (pickNaN(a_cls, b_cls, aIsLargerSignificand, status)) {
-        if (is_snan(b_cls)) {
-            return floatx80_silence_nan(b, status);
-        }
-        return b;
-    } else {
-        if (is_snan(a_cls)) {
-            return floatx80_silence_nan(a, status);
-        }
-        return a;
-    }
+    pr = parts_pick_nan(&pa, &pb, status);
+    return floatx80_round_pack_canonical(pr, status);
 }
 
 /*----------------------------------------------------------------------------
-- 
2.43.0

Re: [PATCH 07/11] softfloat: Use parts_pick_nan in propagateFloatx80NaN

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Unpacking and repacking the parts may be slightly more work
> than we did before, but we get to reuse more code.  For a
> code path handling exceptional values, this is an improvement.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  fpu/softfloat.c | 43 +++++--------------------------------------
>  1 file changed, 5 insertions(+), 38 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 6ba1cfd32a..8de8d5f342 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -4928,48 +4928,15 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
>
>  floatx80 propagateFloatx80NaN(floatx80 a, floatx80 b, float_status *status)

This is a curious function, because although it looks like it
ought to be part of softfloat itself in fact it is used in
exactly one function in target/m68k: floatx80_scale().

>  {
> -    bool aIsLargerSignificand;
> -    FloatClass a_cls, b_cls;
> +    FloatParts128 pa, pb, *pr;
>
> -    /* This is not complete, but is good enough for pickNaN.  */
> -    a_cls = (!floatx80_is_any_nan(a)
> -             ? float_class_normal
> -             : floatx80_is_signaling_nan(a, status)
> -             ? float_class_snan
> -             : float_class_qnan);
> -    b_cls = (!floatx80_is_any_nan(b)
> -             ? float_class_normal
> -             : floatx80_is_signaling_nan(b, status)
> -             ? float_class_snan
> -             : float_class_qnan);
> -
> -    if (is_snan(a_cls) || is_snan(b_cls)) {
> -        float_raise(float_flag_invalid, status);
> -    }
> -
> -    if (status->default_nan_mode) {
> +    if (!floatx80_unpack_canonical(&pa, a, status) ||
> +        !floatx80_unpack_canonical(&pb, b, status)) {
>          return floatx80_default_nan(status);
>      }
>
> -    if (a.low < b.low) {
> -        aIsLargerSignificand = 0;
> -    } else if (b.low < a.low) {
> -        aIsLargerSignificand = 1;
> -    } else {
> -        aIsLargerSignificand = (a.high < b.high) ? 1 : 0;
> -    }
> -
> -    if (pickNaN(a_cls, b_cls, aIsLargerSignificand, status)) {
> -        if (is_snan(b_cls)) {
> -            return floatx80_silence_nan(b, status);
> -        }
> -        return b;
> -    } else {
> -        if (is_snan(a_cls)) {
> -            return floatx80_silence_nan(a, status);
> -        }
> -        return a;
> -    }
> +    pr = parts_pick_nan(&pa, &pb, status);
> +    return floatx80_round_pack_canonical(pr, status);
>  }

If we were using this on anything except m68k this would
be a behaviour change for invalid-encoding floatx80,
but m68k currently makes floatx80_invalid_encoding()
always return false. So I think this should be OK:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[PATCH 08/11] softfloat: Inline pickNaN

[Richard Henderson]

Inline pickNaN into its only caller.  This makes one assert
redundant with the immediately preceding IF.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc      | 82 +++++++++++++++++++++++++----
 fpu/softfloat-specialize.c.inc | 96 ----------------------------------
 2 files changed, 73 insertions(+), 105 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 06cfc6abb5..de51097dcf 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -39,24 +39,88 @@ static void partsN(return_nan)(FloatPartsN *a, float_status *s)
 static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
                                      float_status *s)
 {
+    int cmp, which;
+
     if (is_snan(a->cls) || is_snan(b->cls)) {
         float_raise(float_flag_invalid | float_flag_invalid_snan, s);
     }
 
     if (s->default_nan_mode) {
         parts_default_nan(a, s);
-    } else {
-        int cmp = frac_cmp(a, b);
-        if (cmp == 0) {
-            cmp = a->sign < b->sign;
-        }
+        return a;
+    }
 
-        if (pickNaN(a->cls, b->cls, cmp > 0, s)) {
-            a = b;
-        }
+    cmp = frac_cmp(a, b);
+    if (cmp == 0) {
+        cmp = a->sign < b->sign;
+    }
+
+    switch (s->float_2nan_prop_rule) {
+    case float_2nan_prop_s_ab:
         if (is_snan(a->cls)) {
-            parts_silence_nan(a, s);
+            which = 0;
+        } else if (is_snan(b->cls)) {
+            which = 1;
+        } else if (is_qnan(a->cls)) {
+            which = 0;
+        } else {
+            which = 1;
         }
+        break;
+    case float_2nan_prop_s_ba:
+        if (is_snan(b->cls)) {
+            which = 1;
+        } else if (is_snan(a->cls)) {
+            which = 0;
+        } else if (is_qnan(b->cls)) {
+            which = 1;
+        } else {
+            which = 0;
+        }
+        break;
+    case float_2nan_prop_ab:
+        which = is_nan(a->cls) ? 0 : 1;
+        break;
+    case float_2nan_prop_ba:
+        which = is_nan(b->cls) ? 1 : 0;
+        break;
+    case float_2nan_prop_x87:
+        /*
+         * This implements x87 NaN propagation rules:
+         * SNaN + QNaN => return the QNaN
+         * two SNaNs => return the one with the larger significand, silenced
+         * two QNaNs => return the one with the larger significand
+         * SNaN and a non-NaN => return the SNaN, silenced
+         * QNaN and a non-NaN => return the QNaN
+         *
+         * If we get down to comparing significands and they are the same,
+         * return the NaN with the positive sign bit (if any).
+         */
+        if (is_snan(a->cls)) {
+            if (is_snan(b->cls)) {
+                which = cmp > 0 ? 0 : 1;
+            } else {
+                which = is_qnan(b->cls) ? 1 : 0;
+            }
+        } else if (is_qnan(a->cls)) {
+            if (is_snan(b->cls) || !is_qnan(b->cls)) {
+                which = 0;
+            } else {
+                which = cmp > 0 ? 0 : 1;
+            }
+        } else {
+            which = 1;
+        }
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (which) {
+        a = b;
+    }
+    if (is_snan(a->cls)) {
+        parts_silence_nan(a, s);
     }
     return a;
 }
diff --git a/fpu/softfloat-specialize.c.inc b/fpu/softfloat-specialize.c.inc
index f7a320f6ff..cbbbab52ba 100644
--- a/fpu/softfloat-specialize.c.inc
+++ b/fpu/softfloat-specialize.c.inc
@@ -352,102 +352,6 @@ bool float32_is_signaling_nan(float32 a_, float_status *status)
     }
 }
 
-/*----------------------------------------------------------------------------
-| Select which NaN to propagate for a two-input operation.
-| IEEE754 doesn't specify all the details of this, so the
-| algorithm is target-specific.
-| The routine is passed various bits of information about the
-| two NaNs and should return 0 to select NaN a and 1 for NaN b.
-| Note that signalling NaNs are always squashed to quiet NaNs
-| by the caller, by calling floatXX_silence_nan() before
-| returning them.
-|
-| aIsLargerSignificand is only valid if both a and b are NaNs
-| of some kind, and is true if a has the larger significand,
-| or if both a and b have the same significand but a is
-| positive but b is negative. It is only needed for the x87
-| tie-break rule.
-*----------------------------------------------------------------------------*/
-
-static int pickNaN(FloatClass a_cls, FloatClass b_cls,
-                   bool aIsLargerSignificand, float_status *status)
-{
-    /*
-     * We guarantee not to require the target to tell us how to
-     * pick a NaN if we're always returning the default NaN.
-     * But if we're not in default-NaN mode then the target must
-     * specify via set_float_2nan_prop_rule().
-     */
-    assert(!status->default_nan_mode);
-
-    switch (status->float_2nan_prop_rule) {
-    case float_2nan_prop_s_ab:
-        if (is_snan(a_cls)) {
-            return 0;
-        } else if (is_snan(b_cls)) {
-            return 1;
-        } else if (is_qnan(a_cls)) {
-            return 0;
-        } else {
-            return 1;
-        }
-        break;
-    case float_2nan_prop_s_ba:
-        if (is_snan(b_cls)) {
-            return 1;
-        } else if (is_snan(a_cls)) {
-            return 0;
-        } else if (is_qnan(b_cls)) {
-            return 1;
-        } else {
-            return 0;
-        }
-        break;
-    case float_2nan_prop_ab:
-        if (is_nan(a_cls)) {
-            return 0;
-        } else {
-            return 1;
-        }
-        break;
-    case float_2nan_prop_ba:
-        if (is_nan(b_cls)) {
-            return 1;
-        } else {
-            return 0;
-        }
-        break;
-    case float_2nan_prop_x87:
-        /*
-         * This implements x87 NaN propagation rules:
-         * SNaN + QNaN => return the QNaN
-         * two SNaNs => return the one with the larger significand, silenced
-         * two QNaNs => return the one with the larger significand
-         * SNaN and a non-NaN => return the SNaN, silenced
-         * QNaN and a non-NaN => return the QNaN
-         *
-         * If we get down to comparing significands and they are the same,
-         * return the NaN with the positive sign bit (if any).
-         */
-        if (is_snan(a_cls)) {
-            if (is_snan(b_cls)) {
-                return aIsLargerSignificand ? 0 : 1;
-            }
-            return is_qnan(b_cls) ? 1 : 0;
-        } else if (is_qnan(a_cls)) {
-            if (is_snan(b_cls) || !is_qnan(b_cls)) {
-                return 0;
-            } else {
-                return aIsLargerSignificand ? 0 : 1;
-            }
-        } else {
-            return 1;
-        }
-    default:
-        g_assert_not_reached();
-    }
-}
-
 /*----------------------------------------------------------------------------
 | Returns 1 if the double-precision floating-point value `a' is a quiet
 | NaN; otherwise returns 0.
-- 
2.43.0

Re: [PATCH 08/11] softfloat: Inline pickNaN

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> Inline pickNaN into its only caller.  This makes one assert
> redundant with the immediately preceding IF.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   fpu/softfloat-parts.c.inc      | 82 +++++++++++++++++++++++++----
>   fpu/softfloat-specialize.c.inc | 96 ----------------------------------
>   2 files changed, 73 insertions(+), 105 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 09/11] softfloat: Share code between parts_pick_nan cases

[Richard Henderson]

Remember if there was an SNaN, and use that to simplify
float_2nan_prop_s_{ab,ba} to only the snan component.
Then, fall through to the corresponding
float_2nan_prop_{ab,ba} case to handle any remaining
nans, which must be quiet.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index de51097dcf..099f1c48ef 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -39,10 +39,12 @@ static void partsN(return_nan)(FloatPartsN *a, float_status *s)
 static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
                                      float_status *s)
 {
+    bool have_snan = false;
     int cmp, which;
 
     if (is_snan(a->cls) || is_snan(b->cls)) {
         float_raise(float_flag_invalid | float_flag_invalid_snan, s);
+        have_snan = true;
     }
 
     if (s->default_nan_mode) {
@@ -57,30 +59,20 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
 
     switch (s->float_2nan_prop_rule) {
     case float_2nan_prop_s_ab:
-        if (is_snan(a->cls)) {
-            which = 0;
-        } else if (is_snan(b->cls)) {
-            which = 1;
-        } else if (is_qnan(a->cls)) {
-            which = 0;
-        } else {
-            which = 1;
+        if (have_snan) {
+            which = is_snan(a->cls) ? 0 : 1;
+            break;
         }
-        break;
-    case float_2nan_prop_s_ba:
-        if (is_snan(b->cls)) {
-            which = 1;
-        } else if (is_snan(a->cls)) {
-            which = 0;
-        } else if (is_qnan(b->cls)) {
-            which = 1;
-        } else {
-            which = 0;
-        }
-        break;
+        /* fall through */
     case float_2nan_prop_ab:
         which = is_nan(a->cls) ? 0 : 1;
         break;
+    case float_2nan_prop_s_ba:
+        if (have_snan) {
+            which = is_snan(b->cls) ? 1 : 0;
+            break;
+        }
+        /* fall through */
     case float_2nan_prop_ba:
         which = is_nan(b->cls) ? 1 : 0;
         break;
-- 
2.43.0

Re: [PATCH 09/11] softfloat: Share code between parts_pick_nan cases

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:40, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Remember if there was an SNaN, and use that to simplify
> float_2nan_prop_s_{ab,ba} to only the snan component.
> Then, fall through to the corresponding
> float_2nan_prop_{ab,ba} case to handle any remaining
> nans, which must be quiet.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 10/11] softfloat: Sink frac_cmp in parts_pick_nan until needed

[Richard Henderson]

Move the fractional comparison to the end of the
float_2nan_prop_x87 case.  This is not required for
any other 2nan propagation rule.  Reorganize the
x87 case itself to break out of the switch when the
fractional comparison is not required.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 099f1c48ef..9a2287095c 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -52,11 +52,6 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
         return a;
     }
 
-    cmp = frac_cmp(a, b);
-    if (cmp == 0) {
-        cmp = a->sign < b->sign;
-    }
-
     switch (s->float_2nan_prop_rule) {
     case float_2nan_prop_s_ab:
         if (have_snan) {
@@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
          * return the NaN with the positive sign bit (if any).
          */
         if (is_snan(a->cls)) {
-            if (is_snan(b->cls)) {
-                which = cmp > 0 ? 0 : 1;
-            } else {
+            if (!is_snan(b->cls)) {
                 which = is_qnan(b->cls) ? 1 : 0;
+                break;
             }
         } else if (is_qnan(a->cls)) {
             if (is_snan(b->cls) || !is_qnan(b->cls)) {
                 which = 0;
-            } else {
-                which = cmp > 0 ? 0 : 1;
+                break;
             }
         } else {
             which = 1;
+            break;
         }
+        cmp = frac_cmp(a, b);
+        if (cmp == 0) {
+            cmp = a->sign < b->sign;
+        }
+        which = cmp > 0 ? 0 : 1;
         break;
     default:
         g_assert_not_reached();
-- 
2.43.0

Re: [PATCH 10/11] softfloat: Sink frac_cmp in parts_pick_nan until needed

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:40, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Move the fractional comparison to the end of the
> float_2nan_prop_x87 case.  This is not required for
> any other 2nan propagation rule.  Reorganize the
> x87 case itself to break out of the switch when the
> fractional comparison is not required.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> @@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
>           * return the NaN with the positive sign bit (if any).
>           */
>          if (is_snan(a->cls)) {
> -            if (is_snan(b->cls)) {
> -                which = cmp > 0 ? 0 : 1;
> -            } else {
> +            if (!is_snan(b->cls)) {
>                  which = is_qnan(b->cls) ? 1 : 0;
> +                break;
>              }
>          } else if (is_qnan(a->cls)) {
>              if (is_snan(b->cls) || !is_qnan(b->cls)) {

Pre-existing code, but isn't
   is_snan(X) || !is_qnan(X)
the same as
   !is_qnan(X)
?

thanks
-- PMM

Re: [PATCH 10/11] softfloat: Sink frac_cmp in parts_pick_nan until needed

[Richard Henderson]

On 12/5/24 07:48, Peter Maydell wrote:
> On Tue, 3 Dec 2024 at 20:40, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Move the fractional comparison to the end of the
>> float_2nan_prop_x87 case.  This is not required for
>> any other 2nan propagation rule.  Reorganize the
>> x87 case itself to break out of the switch when the
>> fractional comparison is not required.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
>> @@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
>>            * return the NaN with the positive sign bit (if any).
>>            */
>>           if (is_snan(a->cls)) {
>> -            if (is_snan(b->cls)) {
>> -                which = cmp > 0 ? 0 : 1;
>> -            } else {
>> +            if (!is_snan(b->cls)) {
>>                   which = is_qnan(b->cls) ? 1 : 0;
>> +                break;
>>               }
>>           } else if (is_qnan(a->cls)) {
>>               if (is_snan(b->cls) || !is_qnan(b->cls)) {
> 
> Pre-existing code, but isn't
>     is_snan(X) || !is_qnan(X)
> the same as
>     !is_qnan(X)
> ?

No, since X may not be a NaN at all.  We arrive in pick_nan knowing only that one operand 
must be a NaN, but the other may be anything at all.


r~

Re: [PATCH 10/11] softfloat: Sink frac_cmp in parts_pick_nan until needed

[Richard Henderson]

On 12/5/24 11:51, Richard Henderson wrote:
> On 12/5/24 07:48, Peter Maydell wrote:
>> On Tue, 3 Dec 2024 at 20:40, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> Move the fractional comparison to the end of the
>>> float_2nan_prop_x87 case.  This is not required for
>>> any other 2nan propagation rule.  Reorganize the
>>> x87 case itself to break out of the switch when the
>>> fractional comparison is not required.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>>> @@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
>>>            * return the NaN with the positive sign bit (if any).
>>>            */
>>>           if (is_snan(a->cls)) {
>>> -            if (is_snan(b->cls)) {
>>> -                which = cmp > 0 ? 0 : 1;
>>> -            } else {
>>> +            if (!is_snan(b->cls)) {
>>>                   which = is_qnan(b->cls) ? 1 : 0;
>>> +                break;
>>>               }
>>>           } else if (is_qnan(a->cls)) {
>>>               if (is_snan(b->cls) || !is_qnan(b->cls)) {
>>
>> Pre-existing code, but isn't
>>     is_snan(X) || !is_qnan(X)
>> the same as
>>     !is_qnan(X)
>> ?
> 
> No, since X may not be a NaN at all.  We arrive in pick_nan knowing only that one operand 
> must be a NaN, but the other may be anything at all.

Alternately, and more, correctly, you're right.  :-}


r~

[PATCH 11/11] softfloat: Replace WHICH with RET in parts_pick_nan

[Richard Henderson]

Replace the "index" selecting between A and B with a result variable
of the proper type.  This improves clarity within the function.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 9a2287095c..e51c9827d9 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -40,7 +40,8 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
                                      float_status *s)
 {
     bool have_snan = false;
-    int cmp, which;
+    FloatPartsN *ret;
+    int cmp;
 
     if (is_snan(a->cls) || is_snan(b->cls)) {
         float_raise(float_flag_invalid | float_flag_invalid_snan, s);
@@ -55,21 +56,21 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
     switch (s->float_2nan_prop_rule) {
     case float_2nan_prop_s_ab:
         if (have_snan) {
-            which = is_snan(a->cls) ? 0 : 1;
+            ret = is_snan(a->cls) ? a : b;
             break;
         }
         /* fall through */
     case float_2nan_prop_ab:
-        which = is_nan(a->cls) ? 0 : 1;
+        ret = is_nan(a->cls) ? a : b;
         break;
     case float_2nan_prop_s_ba:
         if (have_snan) {
-            which = is_snan(b->cls) ? 1 : 0;
+            ret = is_snan(b->cls) ? b : a;
             break;
         }
         /* fall through */
     case float_2nan_prop_ba:
-        which = is_nan(b->cls) ? 1 : 0;
+        ret = is_nan(b->cls) ? b : a;
         break;
     case float_2nan_prop_x87:
         /*
@@ -85,35 +86,32 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
          */
         if (is_snan(a->cls)) {
             if (!is_snan(b->cls)) {
-                which = is_qnan(b->cls) ? 1 : 0;
+                ret = is_qnan(b->cls) ? b : a;
                 break;
             }
         } else if (is_qnan(a->cls)) {
             if (is_snan(b->cls) || !is_qnan(b->cls)) {
-                which = 0;
+                ret = a;
                 break;
             }
         } else {
-            which = 1;
+            ret = b;
             break;
         }
         cmp = frac_cmp(a, b);
         if (cmp == 0) {
             cmp = a->sign < b->sign;
         }
-        which = cmp > 0 ? 0 : 1;
+        ret = cmp > 0 ? a : b;
         break;
     default:
         g_assert_not_reached();
     }
 
-    if (which) {
-        a = b;
+    if (is_snan(ret->cls)) {
+        parts_silence_nan(ret, s);
     }
-    if (is_snan(a->cls)) {
-        parts_silence_nan(a, s);
-    }
-    return a;
+    return ret;
 }
 
 static FloatPartsN *partsN(pick_nan_muladd)(FloatPartsN *a, FloatPartsN *b,
-- 
2.43.0

Re: [PATCH 11/11] softfloat: Replace WHICH with RET in parts_pick_nan

[Philippe Mathieu-Daudé]

On 3/12/24 21:39, Richard Henderson wrote:
> Replace the "index" selecting between A and B with a result variable
> of the proper type.  This improves clarity within the function.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   fpu/softfloat-parts.c.inc | 28 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-10.0 00/11] fpu: pickNaN follow ups

[Peter Maydell]

On Tue, 3 Dec 2024 at 20:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Based-on: 20241202131347.498124-55-peter.maydell@linaro.org
> ([PATCH v2 for-10.0 54/54] fpu: Remove default handling for dnan_pattern)
>
> The first patch needs to be inserted into Peter's patch set,
> any place before 54/54 when dnan_pattern becomes mandatory.
>
> The rest are cleanups that become possible when the
> softfloat-specialize.c.inc functions are inlined into their
> single callers in softfloat-parts.c.inc.
>

I've queued this to target-arm.next for 10.0.

thanks
-- PMM