Re: [PATCH v1 01/20] softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag)

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

> Implementation inspired by minmax_floats(). Unfortuantely, we don't have
> any tests we can simply adjust/unlock.
>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Alex Bennée" <alex.bennee@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  fpu/softfloat.c         | 100 ++++++++++++++++++++++++++++++++++++++++
>  include/fpu/softfloat.h |   6 +++
>  2 files changed, 106 insertions(+)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 9af75b9146..9463c5ea56 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -621,6 +621,8 @@ static inline FloatParts float64_unpack_raw(float64 f)
>      return unpack_raw(float64_params, f);
>  }
>  
> +static void float128_unpack(FloatParts128 *p, float128 a, float_status 
> *status);
> +
>  /* Pack a float from parts, but do not canonicalize.  */
>  static inline uint64_t pack_raw(FloatFmt fmt, FloatParts p)
>  {
> @@ -3180,6 +3182,89 @@ static FloatParts minmax_floats(FloatParts a, 
> FloatParts b, bool ismin,
>      }
>  }

It would be desirable to share as much logic for this as possible with
the existing minmax_floats code. I appreciate at some point we end up
having to deal with fractions and we haven't found a good way to
efficiently handle dealing with FloatParts and FloatParts128 in the same
unrolled code, however:

>  
> +static float128 float128_minmax(float128 a, float128 b, bool ismin, bool 
> ieee,
> +                                bool ismag, float_status *s)
> +{
> +    FloatParts128 pa, pb;
> +    int a_exp, b_exp;
> +    bool a_less;
> +
> +    float128_unpack(&pa, a, s);
> +    float128_unpack(&pb, b, s);
> +

>From here:
> +    if (unlikely(is_nan(pa.cls) || is_nan(pb.cls))) {
> +        /* See comment in minmax_floats() */
> +        if (ieee && !is_snan(pa.cls) && !is_snan(pb.cls)) {
> +            if (is_nan(pa.cls) && !is_nan(pb.cls)) {
> +                return b;
> +            } else if (is_nan(pb.cls) && !is_nan(pa.cls)) {
> +                return a;
> +            }
> +        }
> +
> +        /* Similar logic to pick_nan(), avoiding re-packing. */
> +        if (is_snan(pa.cls) || is_snan(pb.cls)) {
> +            s->float_exception_flags |= float_flag_invalid;
> +        }
> +        if (s->default_nan_mode) {
> +            return float128_default_nan(s);
> +        }

to here is common logic - is there anyway we could share it?

> +        if (pickNaN(pa.cls, pb.cls,
> +                    pa.frac0 > pb.frac0 ||
> +                    (pa.frac0 == pb.frac0 && pa.frac1 > pb.frac1) ||
> +                    (pa.frac0 == pb.frac0 && pa.frac1 == pb.frac1 &&
> +                     pa.sign < pb.sign), s)) {
> +            return is_snan(pb.cls) ? float128_silence_nan(b, s) : b;
> +        }
> +        return is_snan(pa.cls) ? float128_silence_nan(a, s) : a;
> +    }
> +
> +    switch (pa.cls) {
> +    case float_class_normal:
> +        a_exp = pa.exp;
> +        break;
> +    case float_class_inf:
> +        a_exp = INT_MAX;
> +        break;
> +    case float_class_zero:
> +        a_exp = INT_MIN;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }

Likewise I wonder if there is scope for a float_minmax_exp helper that
could be shared here?

> +    switch (pb.cls) {
> +    case float_class_normal:
> +        b_exp = pb.exp;
> +        break;
> +    case float_class_inf:
> +        b_exp = INT_MAX;
> +        break;
> +    case float_class_zero:
> +        b_exp = INT_MIN;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +
> +    a_less = a_exp < b_exp;
> +    if (a_exp == b_exp) {
> +        a_less = pa.frac0 < pb.frac0;
> +        if (pa.frac0 == pb.frac0) {
> +            a_less = pa.frac1 < pb.frac1;
> +        }
> +    }
> +
> +    if (ismag &&
> +        (a_exp != b_exp || pa.frac0 != pb.frac0 || pa.frac1 != pb.frac1)) {
> +        return a_less ^ ismin ? b : a;
> +    } else if (pa.sign == pb.sign) {
> +        return pa.sign ^ a_less ^ ismin ? b : a;
> +    }
> +    return pa.sign ^ ismin ? b : a;
> +}

Otherwise it seems sane to me.

-- 
Alex Bennée