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

From: David Hildenbrand <david@redhat.com>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Thomas Huth <thuth@redhat.com>, Cornelia Huck <cohuck@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, qemu-s390x@nongnu.org,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH v1 01/20] softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag)
Date: Thu, 1 Oct 2020 14:40:39 +0200	[thread overview]
Message-ID: <1294d810-64a7-7e4d-fb73-238378e2197c@redhat.com> (raw)
In-Reply-To: <87o8lnnt3m.fsf@linaro.org>

On 30.09.20 18:10, Alex Bennée wrote:
> 
> 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?

I can try to factor it out, similar to pickNaN() - passing weird boolean
flags and such. It most certainly won't win in a beauty contest, that's
for sure.

> 
>> +        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?

I'll try, but I have the feeling that it might make the code harder to
read than actually help. Will give it a try.

Thanks!

-- 
Thanks,

David / dhildenb