Comments

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(-)

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