| Message ID | alpine.DEB.2.21.2005010038260.30535@digraph.polyomino.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | softfloat: fix floatx80 emulation bugs | expand |
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
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 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?
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 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.
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 {
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(+)