[PATCH 0/3] softfloat: FloatRelation cleanups

[PATCH 0/3] softfloat: FloatRelation cleanups

[Richard Henderson]

Make consistent use of FloatRelation throughout the
implementation of the float compare functions.

I don't yet know if this actually solves Coverity issues,
but they all look a bit cleaner, I think.

r~

Richard Henderson (3):
  softfloat: Fix declaration of partsN_compare
  softfloat: Use FloatRelation within partsN_compare
  softfloat: Use FloatRelation for fracN_cmp

 fpu/softfloat.c           | 20 +++++++++++---------
 fpu/softfloat-parts.c.inc | 11 +++++++----
 2 files changed, 18 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH 1/3] softfloat: Fix declaration of partsN_compare

[Richard Henderson]

The declaration used 'int', while the definition used 'FloatRelation'.
This should have resulted in a compiler error, but mysteriously didn't.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 7f524d4377..7e62fcf414 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -874,10 +874,10 @@ static FloatParts128 *parts128_minmax(FloatParts128 *a, FloatParts128 *b,
 #define parts_minmax(A, B, S, F) \
     PARTS_GENERIC_64_128(minmax, A)(A, B, S, F)
 
-static int parts64_compare(FloatParts64 *a, FloatParts64 *b,
-                           float_status *s, bool q);
-static int parts128_compare(FloatParts128 *a, FloatParts128 *b,
-                            float_status *s, bool q);
+static FloatRelation parts64_compare(FloatParts64 *a, FloatParts64 *b,
+                                     float_status *s, bool q);
+static FloatRelation parts128_compare(FloatParts128 *a, FloatParts128 *b,
+                                      float_status *s, bool q);
 
 #define parts_compare(A, B, S, Q) \
     PARTS_GENERIC_64_128(compare, A)(A, B, S, Q)
-- 
2.25.1

[PATCH 2/3] softfloat: Use FloatRelation within partsN_compare

[Richard Henderson]

As the return type is FloatRelation, it's clearer to
use the type for 'cmp' within the function.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index db3e1f393d..bbeadaa189 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -1327,16 +1327,19 @@ static FloatRelation partsN(compare)(FloatPartsN *a, FloatPartsN *b,
                                      float_status *s, bool is_quiet)
 {
     int ab_mask = float_cmask(a->cls) | float_cmask(b->cls);
-    int cmp;
 
     if (likely(ab_mask == float_cmask_normal)) {
+        FloatRelation cmp;
+
         if (a->sign != b->sign) {
             goto a_sign;
         }
-        if (a->exp != b->exp) {
-            cmp = a->exp < b->exp ? -1 : 1;
-        } else {
+        if (a->exp == b->exp) {
             cmp = frac_cmp(a, b);
+        } else if (a->exp < b->exp) {
+            cmp = float_relation_less;
+        } else {
+            cmp = float_relation_greater;
         }
         if (a->sign) {
             cmp = -cmp;
-- 
2.25.1

[PATCH 3/3] softfloat: Use FloatRelation for fracN_cmp

[Richard Henderson]

Since the caller, partsN_compare, is now exclusively
using FloatRelation, it's clearer to use it here too.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 7e62fcf414..9f2d4e7a29 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -957,21 +957,23 @@ static void frac128_allones(FloatParts128 *a)
 
 #define frac_allones(A)  FRAC_GENERIC_64_128(allones, A)(A)
 
-static int frac64_cmp(FloatParts64 *a, FloatParts64 *b)
+static FloatRelation frac64_cmp(FloatParts64 *a, FloatParts64 *b)
 {
-    return a->frac == b->frac ? 0 : a->frac < b->frac ? -1 : 1;
+    return (a->frac == b->frac ? float_relation_equal
+            : a->frac < b->frac ? float_relation_less
+            : float_relation_greater);
 }
 
-static int frac128_cmp(FloatParts128 *a, FloatParts128 *b)
+static FloatRelation frac128_cmp(FloatParts128 *a, FloatParts128 *b)
 {
     uint64_t ta = a->frac_hi, tb = b->frac_hi;
     if (ta == tb) {
         ta = a->frac_lo, tb = b->frac_lo;
         if (ta == tb) {
-            return 0;
+            return float_relation_equal;
         }
     }
-    return ta < tb ? -1 : 1;
+    return ta < tb ? float_relation_less : float_relation_greater;
 }
 
 #define frac_cmp(A, B)  FRAC_GENERIC_64_128(cmp, A)(A, B)
-- 
2.25.1

Re: [PATCH 1/3] softfloat: Fix declaration of partsN_compare

[Peter Maydell]

On Fri, 1 Apr 2022 at 14:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The declaration used 'int', while the definition used 'FloatRelation'.
> This should have resulted in a compiler error, but mysteriously didn't.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  fpu/softfloat.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/3] softfloat: Use FloatRelation within partsN_compare

[Peter Maydell]

On Fri, 1 Apr 2022 at 14:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> As the return type is FloatRelation, it's clearer to
> use the type for 'cmp' within the function.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  fpu/softfloat-parts.c.inc | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/3] softfloat: Use FloatRelation for fracN_cmp

[Peter Maydell]

On Fri, 1 Apr 2022 at 14:22, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Since the caller, partsN_compare, is now exclusively
> using FloatRelation, it's clearer to use it here too.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/3] softfloat: Fix declaration of partsN_compare

[Richard Henderson]

On 4/1/22 07:22, Richard Henderson wrote:
> The declaration used 'int', while the definition used 'FloatRelation'.
> This should have resulted in a compiler error, but mysteriously didn't.

Bah, of course there's no error -- this is C not C++.

The enumeration has values -1 ... 2, which means that the enumeration is compatible with 
an implementation defined signed integer type, which for our set of hosts will be 'int'. 
So, no conflict.

I'll edit the commit message.


r~

Re: [PATCH 1/3] softfloat: Fix declaration of partsN_compare

[Peter Maydell]

On Fri, 1 Apr 2022 at 19:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/1/22 07:22, Richard Henderson wrote:
> > The declaration used 'int', while the definition used 'FloatRelation'.
> > This should have resulted in a compiler error, but mysteriously didn't.
>
> Bah, of course there's no error -- this is C not C++.
>
> The enumeration has values -1 ... 2, which means that the enumeration is compatible with
> an implementation defined signed integer type, which for our set of hosts will be 'int'.
> So, no conflict.

The types are compatible, but it's weird that the compiler doesn't
warn that the prototype and definition are different types: it
seems like the kind of "technically valid but usually a bug" that
a warning would be nice for.

-- PMM

Re: [PATCH 1/3] softfloat: Fix declaration of partsN_compare

[Richard Henderson]

On 4/1/22 12:12, Peter Maydell wrote:
> The types are compatible, but it's weird that the compiler doesn't
> warn that the prototype and definition are different types: it
> seems like the kind of "technically valid but usually a bug" that
> a warning would be nice for.

Good point. Submitted

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105131
https://github.com/llvm/llvm-project/issues/54706


r~

Re: [PATCH 0/3] softfloat: FloatRelation cleanups

[Richard Henderson]

On 4/1/22 06:22, Richard Henderson wrote:
> Make consistent use of FloatRelation throughout the
> implementation of the float compare functions.
> 
> I don't yet know if this actually solves Coverity issues,
> but they all look a bit cleaner, I think.
> 
> r~
> 
> Richard Henderson (3):
>    softfloat: Fix declaration of partsN_compare
>    softfloat: Use FloatRelation within partsN_compare
>    softfloat: Use FloatRelation for fracN_cmp
> 
>   fpu/softfloat.c           | 20 +++++++++++---------
>   fpu/softfloat-parts.c.inc | 11 +++++++----
>   2 files changed, 18 insertions(+), 13 deletions(-)
> 

Queuing to tcg-next.


r~