[PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[Joseph Myers]

The softfloat floatx80 comparisons fail to allow for pseudo-denormals,
which should compare equal to corresponding values with biased
exponent 1 rather than 0.  Add an adjustment for that case when
comparing numbers with the same sign.

Note that this fix only changes floatx80_compare_internal, not the
other more specific comparison operations.  That is the only
comparison function for floatx80 used in the i386 port, which is the
only supported port with these pseudo-denormal semantics.

Signed-off-by: Joseph Myers <joseph@codesourcery.com>
---
 fpu/softfloat.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 6094d267b5..8e9c714e6f 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -7966,6 +7966,11 @@ static inline int floatx80_compare_internal(floatx80 a, floatx80 b,
             return 1 - (2 * aSign);
         }
     } else {
+        /* Normalize pseudo-denormals before comparison.  */
+        if ((a.high & 0x7fff) == 0 && a.low & UINT64_C(0x8000000000000000))
+            ++a.high;
+        if ((b.high & 0x7fff) == 0 && b.low & UINT64_C(0x8000000000000000))
+            ++b.high;
         if (a.low == b.low && a.high == b.high) {
             return float_relation_equal;
         } else {
-- 
2.17.1


-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[Alex Bennée]

Joseph Myers <joseph@codesourcery.com> writes:

> The softfloat floatx80 comparisons fail to allow for pseudo-denormals,
> which should compare equal to corresponding values with biased
> exponent 1 rather than 0.  Add an adjustment for that case when
> comparing numbers with the same sign.
>
> Note that this fix only changes floatx80_compare_internal, not the
> other more specific comparison operations.  That is the only
> comparison function for floatx80 used in the i386 port, which is the
> only supported port with these pseudo-denormal semantics.

Again I can't see anything that triggers this although I noticed
le_quiet has been fixed in the meantime. lt_quiet still fails with:

  ./fp-test -s -l 2 -r all  extF80_lt_quiet
  >> Testing extF80_lt_quiet
  59535872 tests total.
  Errors found in extF80_lt_quiet:
  +0000.0000000000000000  +0000.0000000000000000  => 1 .....  expected 0 .....
  +0000.0000000000000000  -0000.0000000000000000  => 1 .....  expected 0 .....
  +0000.0000000000000001  +0000.0000000000000001  => 1 .....  expected 0 .....
  +0000.0000000000000002  +0000.0000000000000002  => 1 .....  expected 0 .....
  +0000.0000000000000004  +0000.0000000000000004  => 1 .....  expected 0 .....
  +0000.0000000000000008  +0000.0000000000000008  => 1 .....  expected 0 .....
  +0000.0000000000000010  +0000.0000000000000010  => 1 .....  expected 0 .....
  +0000.0000000000000020  +0000.0000000000000020  => 1 .....  expected 0 .....
  +0000.0000000000000040  +0000.0000000000000040  => 1 .....  expected 0 .....
  +0000.0000000000000080  +0000.0000000000000080  => 1 .....  expected 0 .....
  +0000.0000000000000100  +0000.0000000000000100  => 1 .....  expected 0 .....
  +0000.0000000000000200  +0000.0000000000000200  => 1 .....  expected 0 .....
  +0000.0000000000000400  +0000.0000000000000400  => 1 .....  expected 0 .....
  +0000.0000000000000800  +0000.0000000000000800  => 1 .....  expected 0 .....
  +0000.0000000000001000  +0000.0000000000001000  => 1 .....  expected 0 .....
  +0000.0000000000002000  +0000.0000000000002000  => 1 .....  expected 0 .....
  +0000.0000000000004000  +0000.0000000000004000  => 1 .....  expected 0 .....
  +0000.0000000000008000  +0000.0000000000008000  => 1 .....  expected 0 .....
  +0000.0000000000010000  +0000.0000000000010000  => 1 .....  expected 0 .....
  +0000.0000000000020000  +0000.0000000000020000  => 1 .....  expected 0 .....


>
> Signed-off-by: Joseph Myers <joseph@codesourcery.com>
> ---
>  fpu/softfloat.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 6094d267b5..8e9c714e6f 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -7966,6 +7966,11 @@ static inline int floatx80_compare_internal(floatx80 a, floatx80 b,
>              return 1 - (2 * aSign);
>          }
>      } else {
> +        /* Normalize pseudo-denormals before comparison.  */
> +        if ((a.high & 0x7fff) == 0 && a.low & UINT64_C(0x8000000000000000))
> +            ++a.high;
> +        if ((b.high & 0x7fff) == 0 && b.low & UINT64_C(0x8000000000000000))
> +            ++b.high;
>          if (a.low == b.low && a.high == b.high) {
>              return float_relation_equal;
>          } else {
> -- 
> 2.17.1


-- 
Alex Bennée

Re: [PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Fri, 1 May 2020, Alex Bennée wrote:

> 
> Joseph Myers <joseph@codesourcery.com> writes:
> 
> > The softfloat floatx80 comparisons fail to allow for pseudo-denormals,
> > which should compare equal to corresponding values with biased
> > exponent 1 rather than 0.  Add an adjustment for that case when
> > comparing numbers with the same sign.
> >
> > Note that this fix only changes floatx80_compare_internal, not the
> > other more specific comparison operations.  That is the only
> > comparison function for floatx80 used in the i386 port, which is the
> > only supported port with these pseudo-denormal semantics.
> 
> Again I can't see anything that triggers this although I noticed
> le_quiet has been fixed in the meantime. lt_quiet still fails with:

It looks like this test is only testing the separate comparison functions, 
which aren't used in the i386 port and which I didn't change, not anything 
that uses floatx80_compare_internal.  (That's apart from probably not 
covering pseudo-denormals either.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[Alex Bennée]

Joseph Myers <joseph@codesourcery.com> writes:

> On Fri, 1 May 2020, Alex Bennée wrote:
>
>> 
>> Joseph Myers <joseph@codesourcery.com> writes:
>> 
>> > The softfloat floatx80 comparisons fail to allow for pseudo-denormals,
>> > which should compare equal to corresponding values with biased
>> > exponent 1 rather than 0.  Add an adjustment for that case when
>> > comparing numbers with the same sign.
>> >
>> > Note that this fix only changes floatx80_compare_internal, not the
>> > other more specific comparison operations.  That is the only
>> > comparison function for floatx80 used in the i386 port, which is the
>> > only supported port with these pseudo-denormal semantics.
>> 
>> Again I can't see anything that triggers this although I noticed
>> le_quiet has been fixed in the meantime. lt_quiet still fails with:
>
> It looks like this test is only testing the separate comparison functions, 
> which aren't used in the i386 port and which I didn't change, not anything 
> that uses floatx80_compare_internal.  (That's apart from probably not 
> covering pseudo-denormals either.)

OK - so these only turn up in i386?

I think then the things we need for v2 are:

 a) ensure we don't break the existing working TestFloat tests
 b) try an enable the previously broken tests for areas touched
 c) introduce some i386 specific tests to guard the pseudo-denormal
 behaviour

We have two tests currently (float_convs and float_madds) which
currently exercise the various combinations of limits and NaN types
using some common float_helpers.c support. Maybe extend it for have a
table of the various ext80 types and write a i386 only test case to
exercise the functions you fixed?

-- 
Alex Bennée

Re: [PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

On Fri, 1 May 2020, Alex Bennée wrote:

> OK - so these only turn up in i386?

Patch 1, silencing sNaN, is about generic semantics of IEEE floating-point 
conversions (which are implemented correctly in various other cases in 
QEMU), and would be equally applicable to m68k (I believe, without having 
m68k hardware to test).

Patches 2 and 3 are i386-specific (just like everything in the existing 
softfloat code relating to floatx80 subnormals), because m68k interprets 
biased exponent zero differently.

Patch 4 would apply equally to m68k, because all that matters there is 
that a certain representation is a small nonzero value, not exactly what 
value it is.

None of these apply to any other architectures supported by QEMU.

> We have two tests currently (float_convs and float_madds) which
> currently exercise the various combinations of limits and NaN types
> using some common float_helpers.c support. Maybe extend it for have a
> table of the various ext80 types and write a i386 only test case to
> exercise the functions you fixed?

It seems to me that appropriate tests would be entirely i386-specific (in 
tests/tcg/i386?).  How are such tests supposed to signal success or 
failure, since all the tests currently there seem to exit with status 0 
unconditionally?

I do have a test I'm using to check these fixes (in C code for convenience 
of implementation, with only a little inline asm), but it's not suitable 
for inclusion as-is, since it includes many tests that currently fail 
(e.g. for exceptions generated, since the i386 floating-point support in 
QEMU currently discards exceptions from the softfloat code; one of the 
things I intend to fix but haven't yet).  It also doesn't yet cover all 
the problems I think I've found so far in the floating-point support in 
the i386 port (at least ten such bugs beyond the ones fixed in the present 
patch series).  And it might well depend on details of compiler code 
generation to test some of the bugs effectively.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 3/4] softfloat: fix floatx80 pseudo-denormal comparisons

[Alex Bennée]

Joseph Myers <joseph@codesourcery.com> writes:

> On Fri, 1 May 2020, Alex Bennée wrote:
>
>> OK - so these only turn up in i386?
>
> Patch 1, silencing sNaN, is about generic semantics of IEEE floating-point 
> conversions (which are implemented correctly in various other cases in 
> QEMU), and would be equally applicable to m68k (I believe, without having 
> m68k hardware to test).
>
> Patches 2 and 3 are i386-specific (just like everything in the existing 
> softfloat code relating to floatx80 subnormals), because m68k interprets 
> biased exponent zero differently.
>
> Patch 4 would apply equally to m68k, because all that matters there is 
> that a certain representation is a small nonzero value, not exactly what 
> value it is.
>
> None of these apply to any other architectures supported by QEMU.
>
>> We have two tests currently (float_convs and float_madds) which
>> currently exercise the various combinations of limits and NaN types
>> using some common float_helpers.c support. Maybe extend it for have a
>> table of the various ext80 types and write a i386 only test case to
>> exercise the functions you fixed?
>
> It seems to me that appropriate tests would be entirely i386-specific (in 
> tests/tcg/i386?).

Yes.

> How are such tests supposed to signal success or 
> failure, since all the tests currently there seem to exit with status 0 
> unconditionally?

Non-zero exit. The float_convs and madds tests always pass but the
second phase is a diff with a reference output which may fails.
Whichever is easier for your test case.

> I do have a test I'm using to check these fixes (in C code for convenience 
> of implementation, with only a little inline asm), but it's not suitable 
> for inclusion as-is, since it includes many tests that currently fail 
> (e.g. for exceptions generated, since the i386 floating-point support in 
> QEMU currently discards exceptions from the softfloat code; one of the 
> things I intend to fix but haven't yet).  It also doesn't yet cover all 
> the problems I think I've found so far in the floating-point support in 
> the i386 port (at least ten such bugs beyond the ones fixed in the present 
> patch series).  And it might well depend on details of compiler code 
> generation to test some of the bugs effectively.

OK - we certainly want to include tests for fixed functionality as we
add it. It's something we are trying to get better at since the big
re-write a few years ago.

-- 
Alex Bennée