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: Mon, 10 May 2021 12:00:02 +0200	[thread overview]
Message-ID: <5f43bced-2624-9bee-5c92-bd655c1b8410@redhat.com> (raw)
In-Reply-To: <87sg2uyknl.fsf@linaro.org>

On 10.05.21 11:57, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 01.10.20 15:15, Alex Bennée wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> 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.
>>>>>
>>>>> 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.
>>> Give it a try - if it really does become harder to follow then we'll
>>> stick with the duplication however if we can have common code you'll
>>> know at least the nan handling and minmax behaviour for float128 will be
>>> partially tested by the 16/32/64 float code.
>>
>> (finally had time to look into this)
>>
>> What about something like this:
>>
> 
> I was just about to look this morning but I see Richard has posted his
> mega series:
> 
>    From: Richard Henderson <richard.henderson@linaro.org>
>    Subject: [PATCH 00/72] Convert floatx80 and float128 to FloatParts
>    Date: Fri,  7 May 2021 18:46:50 -0700
>    Message-Id: <20210508014802.892561-1-richard.henderson@linaro.org>
> 
> which I think also includes the
> float128_(min|minnum|minnummag|max|maxnum|maxnummag) functions. I'm
> going to have a look at that first if that's ok.

Sure, I have the gut feeling that it follows a similar approach :)


-- 
Thanks,

David / dhildenb