[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 01/20] softfloat: Implement float128_(min|minnum|minnummag
|
From: |
Alex Bennée |
|
Subject: |
Re: [PATCH v1 01/20] softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag) |
|
Date: |
Mon, 10 May 2021 10:57:16 +0100 |
|
User-agent: |
mu4e 1.5.13; emacs 28.0.50 |
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.
>
>
> From dd6cf176c840fc34a095cb2a158032a994aca5ef Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Tue, 29 Sep 2020 14:36:26 +0200
> Subject: [PATCH] softfloat: Implement
> float128_(min|minnum|minnummag|max|maxnum|maxnummag)
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> 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>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> fpu/softfloat.c | 141 ++++++++++++++++++++++++++++++++--------
> include/fpu/softfloat.h | 6 ++
> 2 files changed, 120 insertions(+), 27 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index db7d3a39db..f1014f6d47 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -525,6 +525,18 @@ typedef struct {
> bool sign;
> } FloatParts128;
> +static inline FloatParts128 floatparts64_to_128(FloatParts a)
> +{
> + FloatParts128 res = {
> + .frac0 = a.frac,
> + .exp = a.exp,
> + .cls = a.cls,
> + .sign = a.sign,
> + };
> +
> + return res;
> +}
> +
> #define DECOMPOSED_BINARY_POINT (64 - 2)
> #define DECOMPOSED_IMPLICIT_BIT (1ull << DECOMPOSED_BINARY_POINT)
> #define DECOMPOSED_OVERFLOW_BIT (DECOMPOSED_IMPLICIT_BIT << 1)
> @@ -621,6 +633,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)
> {
> @@ -3093,6 +3107,14 @@ bfloat16 uint16_to_bfloat16(uint16_t a, float_status
> *status)
> return uint64_to_bfloat16_scalbn(a, 0, status);
> }
> +typedef enum MinMaxRes {
> + MINMAX_RES_A,
> + MINMAX_RES_B,
> + MINMAX_RES_SILENCE_A,
> + MINMAX_RES_SILENCE_B,
> + MINMAX_RES_DEFAULT_NAN,
> +} MinMaxRes;
> +
> /* Float Min/Max */
> /* min() and max() functions. These can't be implemented as
> * 'compare and pick one input' because that would mishandle
> @@ -3109,27 +3131,36 @@ bfloat16 uint16_to_bfloat16(uint16_t a, float_status
> *status)
> * minnummag() and maxnummag() functions correspond to minNumMag()
> * and minNumMag() from the IEEE-754 2008.
> */
> -static FloatParts minmax_floats(FloatParts a, FloatParts b, bool ismin,
> - bool ieee, bool ismag, float_status *s)
> +static MinMaxRes minmax_floats128(FloatParts128 a, FloatParts128 b, bool
> ismin,
> + bool ieee, bool ismag, float_status *s)
> {
> if (unlikely(is_nan(a.cls) || is_nan(b.cls))) {
> - if (ieee) {
> - /* Takes two 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.
> - */
> - if (is_snan(a.cls) || is_snan(b.cls)) {
> - return pick_nan(a, b, s);
> - } else if (is_nan(a.cls) && !is_nan(b.cls)) {
> - return b;
> + if (ieee && !is_snan(a.cls) && !is_snan(b.cls)) {
> + if (is_nan(a.cls) && !is_nan(b.cls)) {
> + return MINMAX_RES_B;
> } else if (is_nan(b.cls) && !is_nan(a.cls)) {
> - return a;
> + return MINMAX_RES_A;
> }
> }
> - return pick_nan(a, b, s);
> +
> + /* Similar logic to pick_nan(), avoiding re-packing. */
> + if (is_snan(a.cls) || is_snan(b.cls)) {
> + s->float_exception_flags |= float_flag_invalid;
> + }
> + if (s->default_nan_mode) {
> + return MINMAX_RES_DEFAULT_NAN;
> + }
> + if (pickNaN(a.cls, b.cls,
> + a.frac0 > b.frac0 ||
> + (a.frac0 == b.frac0 && a.frac1 > b.frac1) ||
> + (a.frac0 == b.frac0 && a.frac1 == b.frac1 &&
> + a.sign < b.sign), s)) {
> + return is_snan(b.cls) ? MINMAX_RES_SILENCE_B : MINMAX_RES_B;
> + }
> + return is_snan(a.cls) ? MINMAX_RES_SILENCE_A : MINMAX_RES_A;
> } else {
> int a_exp, b_exp;
> + bool a_less;
> switch (a.cls) {
> case float_class_normal:
> @@ -3160,23 +3191,44 @@ static FloatParts minmax_floats(FloatParts a,
> FloatParts b, bool ismin,
> break;
> }
> - if (ismag && (a_exp != b_exp || a.frac != b.frac)) {
> - bool a_less = a_exp < b_exp;
> - if (a_exp == b_exp) {
> - a_less = a.frac < b.frac;
> + a_less = a_exp < b_exp;
> + if (a_exp == b_exp) {
> + a_less = a.frac0 < b.frac0;
> + if (a.frac0 == b.frac0) {
> + a_less = a.frac1 < b.frac1;
> }
> - return a_less ^ ismin ? b : a;
> }
> - if (a.sign == b.sign) {
> - bool a_less = a_exp < b_exp;
> - if (a_exp == b_exp) {
> - a_less = a.frac < b.frac;
> - }
> - return a.sign ^ a_less ^ ismin ? b : a;
> - } else {
> - return a.sign ^ ismin ? b : a;
> + if (ismag &&
> + (a_exp != b_exp || a.frac0 != b.frac0 || a.frac1 != b.frac1)) {
> + return a_less ^ ismin ? MINMAX_RES_B : MINMAX_RES_A;
> + } else if (a.sign == b.sign) {
> + return a.sign ^ a_less ^ ismin ? MINMAX_RES_B : MINMAX_RES_A;
> }
> + return a.sign ^ ismin ? MINMAX_RES_B : MINMAX_RES_A;
> + }
> +}
> +
> +static FloatParts minmax_floats(FloatParts a, FloatParts b, bool ismin,
> + bool ieee, bool ismag, float_status *s)
> +{
> + FloatParts128 ta = floatparts64_to_128(a);
> + FloatParts128 tb = floatparts64_to_128(b);
> + MinMaxRes res = minmax_floats128(ta, tb, ismin, ieee, ismag, s);
> +
> + switch(res) {
> + case MINMAX_RES_A:
> + return a;
> + case MINMAX_RES_B:
> + return b;
> + case MINMAX_RES_SILENCE_A:
> + return parts_silence_nan(a, s);
> + case MINMAX_RES_SILENCE_B:
> + return parts_silence_nan(b, s);
> + case MINMAX_RES_DEFAULT_NAN:
> + return parts_default_nan(s);
> + default:
> + g_assert_not_reached();
> }
> }
> @@ -3233,6 +3285,41 @@ BF16_MINMAX(maxnummag, false, true, true)
> #undef BF16_MINMAX
> +#define F128_MINMAX(name, ismin, isiee, ismag)
> \
> +float128 float128_ ## name(float128 a, float128 b, float_status *s) \
> +{ \
> + FloatParts128 pa, pb; \
> + MinMaxRes res; \
> + \
> + float128_unpack(&pa, a, s); \
> + float128_unpack(&pb, b, s); \
> + res = minmax_floats128(pa, pb, ismin, isiee, ismag, s); \
> + \
> + switch(res) { \
> + case MINMAX_RES_A: \
> + return a; \
> + case MINMAX_RES_B: \
> + return b; \
> + case MINMAX_RES_SILENCE_A: \
> + return float128_silence_nan(a, s); \
> + case MINMAX_RES_SILENCE_B: \
> + return float128_silence_nan(b, s); \
> + case MINMAX_RES_DEFAULT_NAN: \
> + return float128_default_nan(s); \
> + default: \
> + g_assert_not_reached(); \
> + } \
> +}
> +
> +F128_MINMAX(min, true, false, false)
> +F128_MINMAX(minnum, true, true, false)
> +F128_MINMAX(minnummag, true, true, true)
> +F128_MINMAX(max, false, false, false)
> +F128_MINMAX(maxnum, false, true, false)
> +F128_MINMAX(maxnummag, false, true, true)
> +
> +#undef F128_MINMAX
> +
> /* Floating point compare */
> static FloatRelation compare_floats(FloatParts a, FloatParts b, bool
> is_quiet,
> float_status *s)
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index a38433deb4..4fab2ef6f4 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -1201,6 +1201,12 @@ float128 float128_muladd(float128, float128, float128,
> int,
> float128 float128_sqrt(float128, float_status *status);
> FloatRelation float128_compare(float128, float128, float_status *status);
> FloatRelation float128_compare_quiet(float128, float128, float_status
> *status);
> +float128 float128_min(float128, float128, float_status *status);
> +float128 float128_max(float128, float128, float_status *status);
> +float128 float128_minnum(float128, float128, float_status *status);
> +float128 float128_maxnum(float128, float128, float_status *status);
> +float128 float128_minnummag(float128, float128, float_status *status);
> +float128 float128_maxnummag(float128, float128, float_status *status);
> bool float128_is_quiet_nan(float128, float_status *status);
> bool float128_is_signaling_nan(float128, float_status *status);
> float128 float128_silence_nan(float128, float_status *status);
> --
> 2.30.2
--
Alex Bennée