| Message ID | 1290538431-13170-9-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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~
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
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. */
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(-)