Patchwork [08/12] ARM: Return correct result for single<->double conversion of NaN

login
register
mail settings
Submitter Peter Maydell
Date Nov. 23, 2010, 6:53 p.m.
Message ID <1290538431-13170-9-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/72736/
State New
Headers show

Comments

Peter Maydell - Nov. 23, 2010, 6:53 p.m.
The ARM ARM defines that if the input to a single<->double conversion
is a NaN then the output is always forced to be a quiet NaN by setting
the most significant bit of the fraction part.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
Nathan Froyd - Nov. 29, 2010, 5:49 p.m.
On Tue, Nov 23, 2010 at 06:53:47PM +0000, Peter Maydell wrote:
> The ARM ARM defines that if the input to a single<->double conversion
> is a NaN then the output is always forced to be a quiet NaN by setting
> the most significant bit of the fraction part.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> @@ -2529,12 +2529,26 @@ float32 VFP_HELPER(tosiz, d)(float64 x, CPUState *env)
>  /* floating point conversion */
>  float64 VFP_HELPER(fcvtd, s)(float32 x, CPUState *env)
>  {
> -    return float32_to_float64(x, &env->vfp.fp_status);
> +    float64 r = float32_to_float64(x, &env->vfp.fp_status);
> +    /* ARM requires that S<->D conversion of any kind of NaN generates
> +     * a quiet NaN by forcing the most significant frac bit to 1.
> +     */
> +    if (float64_is_signaling_nan(r)) {
> +        return make_float64(float64_val(r) | (1LL << 51));
> +    }
> +    return r;
>  }

As with other NaN-handling patches, I don't think the bit-twiddling here
is a good idea.  Having a float*_maybe_silence_nan function in softfloat
would be a better approach.

-Nathan
Peter Maydell - Nov. 29, 2010, 7:25 p.m.
On 29 November 2010 17:49, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Tue, Nov 23, 2010 at 06:53:47PM +0000, Peter Maydell wrote:
>> +    /* ARM requires that S<->D conversion of any kind of NaN generates
>> +     * a quiet NaN by forcing the most significant frac bit to 1.
>> +     */
>> +    if (float64_is_signaling_nan(r)) {
>> +        return make_float64(float64_val(r) | (1LL << 51));
>> +    }

> As with other NaN-handling patches, I don't think the bit-twiddling here
> is a good idea.  Having a float*_maybe_silence_nan function in softfloat
> would be a better approach.

I guess this (like the other one you commented on) boils down to how
you want to approach the boundary between qemu proper and the
softfloat library. There are three approaches I can see:

(a) live with the softfloat API as it is, and add bit twiddling as
necessary for particular target CPU special casing in the per-cpu
functions (which is what I was doing here and with the 'is it a NaN?'
function in the other patch)
(b) add to and extend the softfloat API whenever you have some
floating-point related thing it doesn't currently support (which I
did with the "add conversions to int16_t" patch because it was
a big chunk of bit twiddling, but which I felt was a bit invasive to
do for this sort of minor tweak, especially since softfloat is a
copy of a third-party library)
(c) do something suboptimal where the softfloat API provides
some-API-but-not-quite-the-ideal-API (which I'm not particularly
keen on and is what I see the "is_nan() || is_signalling_nan()"
approach as)

My original patchset tends to (a) except where (b) is clearly
more sensible; if people would prefer (b) all the time I'm happy
to do things that way; (c) doesn't seem very attractive to me
and I would rather do (b) in those situations.

-- PMM
Nathan Froyd - Nov. 29, 2010, 7:54 p.m.
On Mon, Nov 29, 2010 at 07:25:18PM +0000, Peter Maydell wrote:
> On 29 November 2010 17:49, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > On Tue, Nov 23, 2010 at 06:53:47PM +0000, Peter Maydell wrote:
> > As with other NaN-handling patches, I don't think the bit-twiddling here
> > is a good idea.  Having a float*_maybe_silence_nan function in softfloat
> > would be a better approach.
> 
> I guess this (like the other one you commented on) boils down to how
> you want to approach the boundary between qemu proper and the
> softfloat library. There are three approaches I can see:
> 
> (a) live with the softfloat API as it is, and add bit twiddling as
> necessary for particular target CPU special casing in the per-cpu
> functions (which is what I was doing here and with the 'is it a NaN?'
> function in the other patch)

Full disclosure: I did this sort of thing for PPC; see the DO_HANDLE_NAN
macro in op_helper.c.  I was young and thoughtless then and now I
am...well, older, anyway. :) I don't think it's the best approach: since
at least two CPUs now need NaN-silencing, we should provide something
generic.  And even if only one CPU requires it, I think it's better to
squirrel the logic away in fpu/.  float{32,64,80,128} should be opaque
things except for specialized cases.

(I can see an argument for CPU-confined bit twiddling to implement
things like float*_rsqrt_estimate or similar, where one function might
not work across CPU families due to precision requirements and so forth.
But we can cross that bridge when we come to it.)

> (b) add to and extend the softfloat API whenever you have some
> floating-point related thing it doesn't currently support (which I
> did with the "add conversions to int16_t" patch because it was
> a big chunk of bit twiddling, but which I felt was a bit invasive to
> do for this sort of minor tweak, especially since softfloat is a
> copy of a third-party library)

I think this is the best approach whenever possible.  I would not be too
worried about the third-party-ness of softfloat; it's extremely stable,
unlikely to change anytime in the near or far future, and we've already
extended in it non-trivial ways anyway.  (And would do so again if we
ever implemented, say, proper flush-to-zero denormal handling or IA64
register-precision floats--the former being more likely than the
latter. ;)

An example of where softfloat could be usefully extended and where we do
(a) sometimes is in production of CPU-default NaN values.  softfloat
knows all about this, yet there are #defines scattered about to provide
bit patterns because the softfloat API isn't extensive enough.

> (c) do something suboptimal where the softfloat API provides
> some-API-but-not-quite-the-ideal-API (which I'm not particularly
> keen on and is what I see the "is_nan() || is_signalling_nan()"
> approach as)

Yes, this is ugly.  Are you up for running:

  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c

(and also carefully in fpu/*) or similar and moving the bit-twiddling
float_is_nan into fpu/?

-Nathan
Peter Maydell - Nov. 29, 2010, 8:04 p.m.
On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Mon, Nov 29, 2010 at 07:25:18PM +0000, Peter Maydell wrote:
>> (b) add to and extend the softfloat API whenever you have some
>> floating-point related thing it doesn't currently support
>
> I think this is the best approach whenever possible.  I would not be too
> worried about the third-party-ness of softfloat; it's extremely stable,
> unlikely to change anytime in the near or far future, and we've already
> extended in it non-trivial ways anyway.

OK. Do we care about maintaining consistency of the API between
softfloat and softfloat-native (the latter used only on x86, x86_64,
cris, sh4, sh4eb)? I didn't in these patchsets because there didn't
seem any point adding functions to softfloat-native that weren't
going to be used by anything and so couldn't be tested.

>> (c) do something suboptimal where the softfloat API provides
>> some-API-but-not-quite-the-ideal-API (which I'm not particularly
>> keen on and is what I see the "is_nan() || is_signalling_nan()"
>> approach as)
>
> Yes, this is ugly.  Are you up for running:
>
>  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
>
> (and also carefully in fpu/*) or similar and moving the bit-twiddling
> float_is_nan into fpu/?

I'm happy to produce a patch doing that if it will be committed :-)

-- PMM
Nathan Froyd - Nov. 29, 2010, 8:21 p.m.
On Mon, Nov 29, 2010 at 08:04:42PM +0000, Peter Maydell wrote:
> On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > On Mon, Nov 29, 2010 at 07:25:18PM +0000, Peter Maydell wrote:
> >> (b) add to and extend the softfloat API whenever you have some
> >> floating-point related thing it doesn't currently support
> >
> > I think this is the best approach whenever possible.
> 
> OK. Do we care about maintaining consistency of the API between
> softfloat and softfloat-native (the latter used only on x86, x86_64,
> cris, sh4, sh4eb)?

softfloat-native should just go away.  I would not worry about API
compatibility between native and non-native configurations there.

> >> (c) do something suboptimal where the softfloat API provides
> >> some-API-but-not-quite-the-ideal-API (which I'm not particularly
> >> keen on and is what I see the "is_nan() || is_signalling_nan()"
> >> approach as)
> >
> > Yes, this is ugly.  Are you up for running:
> >
> >  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
> >
> > (and also carefully in fpu/*) or similar and moving the bit-twiddling
> > float_is_nan into fpu/?
> 
> I'm happy to produce a patch doing that if it will be committed :-)

Well, I can't promise the committal part... :)

-Nathan
Peter Maydell - Nov. 30, 2010, 11:15 a.m.
On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> Yes, this is ugly.  Are you up for running:
>
>  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
>
> (and also carefully in fpu/*) or similar and moving the bit-twiddling
> float_is_nan into fpu/?

I'm just compiling this up now. While I was eyeballing the results of
the substitution, I noticed that there are some places which are almost
certainly bugs introduced by other people not noticing that float*_is_nan()
doesn't do what it says on the tin. Three at random:

In target-ppc/op_helper.c:helper_compute_fprf():
    if (unlikely(float64_is_nan(farg.d))) {
        if (float64_is_signaling_nan(farg.d)) {
            /* Signaling NaN: flags are undefined */
            ret = 0x00;
        } else {
            /* Quiet NaN */
            ret = 0x11;
        }
    } else ...

the 'signalling NaN' case can never be taken.

In target-alpha/op_helper.c:helper_cmptun():
    if (float64_is_nan(fa) || float64_is_nan(fb))
        return 0x4000000000000000ULL;
    else
        return 0;

...but the web suggests this is supposed to return non-zero for any
unordered comparison, not just ones with quiet NaNs.

In target-m68k/helper.c:sub_cmp_f64:
    res = float64_sub(a, b, &env->fp_status);
    if (float64_is_nan(res)) {
        /* +/-inf compares equal against itself, but sub returns nan.  */
        if (!float64_is_nan(a)
            && !float64_is_nan(b)) {
            res = float64_zero;
            if (float64_lt_quiet(a, res, &env->fp_status))
                res = float64_chs(res);
        }
    }

judging from the comments the author expected is_nan() to
be true for all NaNs.

I'm not sure what we should do about these -- they look wrong
but I don't have any of the setup to actually test whether they're
wrong. (The only ARM ones are in the Linux user nwfpe emulation
which is pretty much dead by now IMHO.)

-- PMM
Nathan Froyd - Dec. 1, 2010, 3:39 p.m.
On Tue, Nov 30, 2010 at 11:15:56AM +0000, Peter Maydell wrote:
> On 29 November 2010 19:54, Nathan Froyd <froydnj@codesourcery.com> wrote:
> > Yes, this is ugly.  Are you up for running:
> >
> >  perl -p -i -e 's/float(\d+)_is_nan/float\1_is_quiet_nan/g' target-*/*.c
> >
> > (and also carefully in fpu/*) or similar and moving the bit-twiddling
> > float_is_nan into fpu/?
> 
> I'm just compiling this up now. While I was eyeballing the results of
> the substitution, I noticed that there are some places which are almost
> certainly bugs introduced by other people not noticing that float*_is_nan()
> doesn't do what it says on the tin. Three at random:
> 
> In target-ppc/op_helper.c:helper_compute_fprf():
> 
> In target-alpha/op_helper.c:helper_cmptun():
> 
> In target-m68k/helper.c:sub_cmp_f64:
> 
> judging from the comments the author expected is_nan() to
> be true for all NaNs.
> 
> I'm not sure what we should do about these -- they look wrong
> but I don't have any of the setup to actually test whether they're
> wrong.

RTH (CC'd) is the expert on the Alpha bits.  The PPC one is obviously
wrong.  I can test the m68k one.

In any event, I think the safest course is to do the simple renaming; we
can clean up broken bits after the fact.

-Nathan
Richard Henderson - Dec. 1, 2010, 5:52 p.m.
On 12/01/2010 07:39 AM, Nathan Froyd wrote:
> RTH (CC'd) is the expert on the Alpha bits.

The Alpha cmptun is supposed to return true for both Q+SNaN.

Although, Invalid Operand is supposed to be raised for SNaN,
which is not happening here in helper_cmptun.  Or, indeed,
any of the comparison helper functions.

I think I've lost the thread a bit -- is the proposal to
replace the existing float*_is_nan with _is_quiet_nan and
invent a new function that returns true for both Q+S?  That
at least would be monotonic improvement for Alpha, although
as noted above not 100% correct.


r~
Nathan Froyd - Dec. 1, 2010, 5:54 p.m.
On Wed, Dec 01, 2010 at 09:52:13AM -0800, Richard Henderson wrote:
> I think I've lost the thread a bit -- is the proposal to
> replace the existing float*_is_nan with _is_quiet_nan and
> invent a new function that returns true for both Q+S?  That
> at least would be monotonic improvement for Alpha, although
> as noted above not 100% correct.

Yes, that's the plan.  As (planned to be) implemented, doing this should
not change anything:

1. s/float*_is_nan/float*_is_quiet_nan/g
2. write new float*_is_nan
3. use new float*_is_nan in new code
4. replace bogus float*_is_quiet_nan uses with new function

-Nathan

Patch

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 72ba314..356715c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2529,12 +2529,26 @@  float32 VFP_HELPER(tosiz, d)(float64 x, CPUState *env)
 /* floating point conversion */
 float64 VFP_HELPER(fcvtd, s)(float32 x, CPUState *env)
 {
-    return float32_to_float64(x, &env->vfp.fp_status);
+    float64 r = float32_to_float64(x, &env->vfp.fp_status);
+    /* ARM requires that S<->D conversion of any kind of NaN generates
+     * a quiet NaN by forcing the most significant frac bit to 1.
+     */
+    if (float64_is_signaling_nan(r)) {
+        return make_float64(float64_val(r) | (1LL << 51));
+    }
+    return r;
 }
 
 float32 VFP_HELPER(fcvts, d)(float64 x, CPUState *env)
 {
-    return float64_to_float32(x, &env->vfp.fp_status);
+    float32 r =  float64_to_float32(x, &env->vfp.fp_status);
+    /* ARM requires that S<->D conversion of any kind of NaN generates
+     * a quiet NaN by forcing the most significant frac bit to 1.
+     */
+    if (float32_is_signaling_nan(r)) {
+        return make_float32(float32_val(r) | (1 << 22));
+    }
+    return r;
 }
 
 /* VFP3 fixed point conversion.  */