qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Hildenbrand
Subject: Re: [PATCH v1 01/20] softfloat: Implement float128_(min|minnum|minnummag|max|maxnum|maxnummag)
Date: Mon, 10 May 2021 12:00:02 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]