| Message ID | 52694821.4040001@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 10/24/2013 09:17 AM, Tom Musta wrote: > This patch adds routines to the softfloat library that are useful for > the PowerPC VSX implementation. The routines are, however, not specific > to PowerPC and are approprriate for softfloat. > > The following routines are added: > > - float32_is_denormal() returns true if the 32-bit floating point number > is denormalized. > - float64_is_denormal() returns true if the 64-bit floating point number > is denormalized. > - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit > floating point number. > - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit > floating point number. > - float32_to_uint64() converts a 32-bit floating point number to an > unsigned 64 bit number. Reviewed-by: Richard Henderson <rth@twiddle.net> r~
tommusta@gmail.com writes: > This patch adds routines to the softfloat library that are useful for > the PowerPC VSX implementation. The routines are, however, not specific > to PowerPC and are approprriate for softfloat. <snip> Is it worth adding some sort of test into make check to defend these softfloat functions against unintentional breakage? It would certainly be worthwhile as soon as multiple arches use these functions as float errors are often subtle and hard to track down.
On 25 October 2013 12:34, Alex Bennée <alex.bennee@linaro.org> wrote: > Is it worth adding some sort of test into make check to defend these > softfloat functions against unintentional breakage? It would certainly > be worthwhile as soon as multiple arches use these functions as float > errors are often subtle and hard to track down. Ideally, but there's zero infrastructure for doing the kind of serious including-edge-cases testing at the moment, so I'm not really in favour of making it a gating condition for accepting patches. If somebody wanted to set up such infrastructure, there are a couple of approaches that spring to mind: (a) get risu (https://wiki.linaro.org/PeterMaydell/Risu) working on more target architectures, add the "record-and-replay" feature so it can be run without having target hardware, and then just test softfloat by testing the actual target fp instructions (b) something involving wiring up IBM's IEEE test suite vectors directly to our softfloat code: https://www.research.ibm.com/cgi-bin/haifa/test_suite_download.pl?first=elenag&second=webmaster (it's not clear to me what license the test vectors are under) -- PMM
On 24 October 2013 17:17, Tom Musta <tommusta@gmail.com> wrote: > This patch adds routines to the softfloat library that are useful for > the PowerPC VSX implementation. The routines are, however, not specific > to PowerPC and are approprriate for softfloat. > > The following routines are added: > > - float32_is_denormal() returns true if the 32-bit floating point number > is denormalized. > - float64_is_denormal() returns true if the 64-bit floating point number > is denormalized. Can you point me at the patches which use these, please? I couldn't find them with a quick search in my email client. > - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit > floating point number. > - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit > floating point number. These look rather odd to me, and again I can't find the uses in your patchset. Returning just the exponent is a bit odd and suggests that maybe the split between target code and softfloat is in the wrong place. > - float32_to_uint64() converts a 32-bit floating point number to an > unsigned 64 bit number. I would put this in its own patch, personally. > > +INLINE int float32_is_denormal(float32 a) > +{ > + return ((float32_val(a) & 0x7f800000) == 0) && > + ((float32_val(a) & 0x007fffff) != 0); > +} return float32_is_zero_or_denormal(a) && !float32_is_zero(a); is easier to review and less duplicative of code. thanks -- PMM
Peter: Thanks for your feedback. Responses below. On 10/25/2013 6:55 AM, Peter Maydell wrote: > On 24 October 2013 17:17, Tom Musta <tommusta@gmail.com> wrote: >> This patch adds routines to the softfloat library that are useful for >> the PowerPC VSX implementation. The routines are, however, not specific >> to PowerPC and are approprriate for softfloat. >> >> The following routines are added: >> >> - float32_is_denormal() returns true if the 32-bit floating point number >> is denormalized. >> - float64_is_denormal() returns true if the 64-bit floating point number >> is denormalized. > > Can you point me at the patches which use these, please? > I couldn't find them with a quick search in my email client. Please see http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html > >> - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit >> floating point number. >> - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit >> floating point number. > > These look rather odd to me, and again I can't find the uses in > your patchset. Returning just the exponent is a bit odd and > suggests that maybe the split between target code and softfloat > is in the wrong place. Please see http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html and http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03107.html and also the corresponding definitions of those instructions in the Power ISA. What is odd here is the PowerPC instruction(s) :) But given that softfloat code extracts exponents in numerous places, I do not find it odd at all that a floating point instruction model for a non-standard operation might have to do the same. These functions can easily be kept within the PowerPC code proper if there are objections to them being added to softfloat. I would rename them, of course, so that they do not look like softfloat routines. >> - float32_to_uint64() converts a 32-bit floating point number to an >> unsigned 64 bit number. > > I would put this in its own patch, personally. Fair enough. Just so that I am clear ... do you mean submit this as a patch just by itself (not as part of a series of VSX additions)? >> >> +INLINE int float32_is_denormal(float32 a) >> +{ >> + return ((float32_val(a) & 0x7f800000) == 0) && >> + ((float32_val(a) & 0x007fffff) != 0); >> +} > > return float32_is_zero_or_denormal(a) && !float32_is_zero(a); > > is easier to review and less duplicative of code. > > thanks It surprised me that there were is_zero and is_zero_or_denormal functions but not is_denormal functions. I would find it more normal to implement the two primitive functions and then construct is_zero_or_denormal to be the OR of those two. Until you look at efficiency of the implementation.
peter.maydell@linaro.org writes: > On 25 October 2013 12:34, Alex Bennée <alex.bennee@linaro.org> wrote: >> Is it worth adding some sort of test into make check to defend these >> softfloat functions against unintentional breakage? It would certainly >> be worthwhile as soon as multiple arches use these functions as float >> errors are often subtle and hard to track down. > > Ideally, but there's zero infrastructure for doing the kind > of serious including-edge-cases testing at the moment, so I'm > not really in favour of making it a gating condition for > accepting patches. I'm not proposing to halt inclusion for that I was just wondering aloud how it could be defended. For the soft-float routines themselves they could be tested within the existing tests/ stuff like tests/check-qfloat.c without having to worry about hooking into target arch specific test cases. > If somebody wanted to set up such infrastructure, there are > a couple of approaches that spring to mind: > (a) get risu (https://wiki.linaro.org/PeterMaydell/Risu) working > on more target architectures, add the "record-and-replay" feature > so it can be run without having target hardware, and then just > test softfloat by testing the actual target fp instructions Interesting. Funnily we spent a lot of time at Transitive fixing up translation failures that our random code generator threw up. It's also equally interesting how far you can get with fairly broken translation that no actual applications care about. I'll have a look once I've fixed up build machinery around the existing TCG tests. > (b) something involving wiring up IBM's IEEE test suite > vectors directly to our softfloat code: > https://www.research.ibm.com/cgi-bin/haifa/test_suite_download.pl?first=elenag&second=webmaster > (it's not clear to me what license the test vectors are > under) > > -- PMM
On 10/25/2013 6:44 AM, Peter Maydell wrote: > On 25 October 2013 12:34, Alex Bennée <alex.bennee@linaro.org> wrote: >> Is it worth adding some sort of test into make check to defend these >> softfloat functions against unintentional breakage? It would certainly >> be worthwhile as soon as multiple arches use these functions as float >> errors are often subtle and hard to track down. > > Ideally, but there's zero infrastructure for doing the kind > of serious including-edge-cases testing at the moment, so I'm > not really in favour of making it a gating condition for > accepting patches. > > If somebody wanted to set up such infrastructure, there are > a couple of approaches that spring to mind: > (a) get risu (https://wiki.linaro.org/PeterMaydell/Risu) working > on more target architectures, add the "record-and-replay" feature > so it can be run without having target hardware, and then just > test softfloat by testing the actual target fp instructions > (b) something involving wiring up IBM's IEEE test suite > vectors directly to our softfloat code: > https://www.research.ibm.com/cgi-bin/haifa/test_suite_download.pl?first=elenag&second=webmaster > (it's not clear to me what license the test vectors are > under) Softfloat would seem to lend itself very well to unit testing which makes (b) attractive. Let me see if I can get an answer to the licensing question.
On 25 October 2013 14:01, Tom Musta <tommusta@gmail.com> wrote: > On 10/25/2013 6:55 AM, Peter Maydell wrote: >> On 24 October 2013 17:17, Tom Musta <tommusta@gmail.com> wrote: >>> - float32_is_denormal() returns true if the 32-bit floating point >>> number >>> is denormalized. >>> - float64_is_denormal() returns true if the 64-bit floating point >>> number >>> is denormalized. >> >> >> Can you point me at the patches which use these, please? >> I couldn't find them with a quick search in my email client. > > > Please see > http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html Thanks. For that code you can just use the existing is_zero_or_denormal function if you like, since you've already ruled out "is this zero?" by the time you're checking for "is this denormal?". (In fact that logic seems to do a number of pointless checks for "is this zero?" when it's already ruled that case out very early; it should probably be rephrased.) However I don't think there's any harm in our providing some *_is_denormal() functions in our softfloat API if the code seems clearer if it's written to use them. It does fill out an odd gap in the API shape, as you note below. >>> - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit >>> floating point number. >>> - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit >>> floating point number. >> >> >> These look rather odd to me, and again I can't find the uses in >> your patchset. Returning just the exponent is a bit odd and >> suggests that maybe the split between target code and softfloat >> is in the wrong place. > > > Please see > http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03108.html > and http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg03107.html > and also the corresponding definitions of those instructions in the Power > ISA. > > What is odd here is the PowerPC instruction(s) :) > > But given that softfloat code extracts exponents in numerous places, I do > not find > it odd at all that a floating point instruction model for a non-standard > operation might have to do the same. > > These functions can easily be kept within the PowerPC code proper if there > are > objections to them being added to softfloat. I would rename them, of > course, so > that they do not look like softfloat routines. Mmm. You'll notice that your calling code has to know rather a lot about the format of the IEEE floats (in that it has to know the min/max exponent and mantissa width). So I think I'd just opencode these in the PPC routines. (This is what we do in target-arm, see recpe_f32 and rsqrte_f32 for examples.) >>> - float32_to_uint64() converts a 32-bit floating point number to an >>> unsigned 64 bit number. >> >> >> I would put this in its own patch, personally. > > > Fair enough. Just so that I am clear ... do you mean submit this as a patch > just by itself (not as part of a series of VSX additions)? I mean "in its own patch email so it is a separate commit and clearly separated from other things for code review purposes". You probably still keep it as part of this patch series. (In fact it would also be a good idea to include the previous patch this one depends on, if that has not yet been committed.) > >>> >>> +INLINE int float32_is_denormal(float32 a) >>> +{ >>> + return ((float32_val(a) & 0x7f800000) == 0) && >>> + ((float32_val(a) & 0x007fffff) != 0); >>> +} >> >> >> return float32_is_zero_or_denormal(a) && !float32_is_zero(a); >> >> is easier to review and less duplicative of code. >> >> thanks > > > It surprised me that there were is_zero and is_zero_or_denormal functions > but > not is_denormal functions. I would find it more normal to implement the two > primitive functions and then construct is_zero_or_denormal to be the OR of > those two. Until you look at efficiency of the implementation. I think also the original uses of these functions didn't need to distinguish zero from denormal, so it was a more natural API for those uses. -- PMM
diff --git a/fpu/softfloat.c b/fpu/softfloat.c index 3070eaa..cb03dca 100644 --- a/fpu/softfloat.c +++ b/fpu/softfloat.c @@ -1550,6 +1550,51 @@ int64 float32_to_int64( float32 a STATUS_PARAM ) /*---------------------------------------------------------------------------- | Returns the result of converting the single-precision floating-point value +| `a' to the 64-bit unsigned integer format. The conversion is +| performed according to the IEC/IEEE Standard for Binary Floating-Point +| Arithmetic---which means in particular that the conversion is rounded +| according to the current rounding mode. If `a' is a NaN, the largest +| unsigned integer is returned. Otherwise, if the conversion overflows, the +| largest unsigned integer is returned. If the 'a' is negative, zero is +| returned. +*----------------------------------------------------------------------------*/ + +uint64 float32_to_uint64(float32 a STATUS_PARAM) +{ + flag aSign; + int_fast16_t aExp, shiftCount; + uint32_t aSig; + uint64_t aSig64, aSigExtra; + a = float32_squash_input_denormal(a STATUS_VAR); + + aSig = extractFloat32Frac(a); + aExp = extractFloat32Exp(a); + aSign = extractFloat32Sign(a); + if (aSign) { + if (aExp) { + float_raise(float_flag_invalid STATUS_VAR); + } else if (aSig) { /* negative denormalized */ + float_raise(float_flag_inexact STATUS_VAR); + } + return 0; + } + shiftCount = 0xBE - aExp; + if (aExp) { + aSig |= 0x00800000; + } + if (shiftCount < 0) { + float_raise(float_flag_invalid STATUS_VAR); + return (int64_t)LIT64(0xFFFFFFFFFFFFFFFF); + } + + aSig64 = aSig; + aSig64 <<= 40; + shift64ExtraRightJamming(aSig64, 0, shiftCount, &aSig64, &aSigExtra); + return roundAndPackUint64(aSig64, aSigExtra STATUS_VAR); +} + +/*---------------------------------------------------------------------------- +| Returns the result of converting the single-precision floating-point value | `a' to the 64-bit two's complement integer format. The conversion is | performed according to the IEC/IEEE Standard for Binary Floating-Point | Arithmetic, except that the conversion is always rounded toward zero. If diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h index f3927e2..678e527 100644 --- a/include/fpu/softfloat.h +++ b/include/fpu/softfloat.h @@ -272,6 +272,7 @@ int32 float32_to_int32_round_to_zero( float32 STATUS_PARAM ); uint32 float32_to_uint32( float32 STATUS_PARAM ); uint32 float32_to_uint32_round_to_zero( float32 STATUS_PARAM ); int64 float32_to_int64( float32 STATUS_PARAM ); +uint64 float32_to_uint64(float32 STATUS_PARAM); int64 float32_to_int64_round_to_zero( float32 STATUS_PARAM ); float64 float32_to_float64( float32 STATUS_PARAM ); floatx80 float32_to_floatx80( float32 STATUS_PARAM ); @@ -348,6 +349,12 @@ INLINE int float32_is_zero_or_denormal(float32 a) return (float32_val(a) & 0x7f800000) == 0; } +INLINE int float32_is_denormal(float32 a) +{ + return ((float32_val(a) & 0x7f800000) == 0) && + ((float32_val(a) & 0x007fffff) != 0); +} + INLINE float32 float32_set_sign(float32 a, int sign) { return make_float32((float32_val(a) & 0x7fffffff) | (sign << 31)); @@ -360,6 +367,10 @@ INLINE float32 float32_set_sign(float32 a, int sign) #define float32_half make_float32(0x3f000000) #define float32_infinity make_float32(0x7f800000) +INLINE int float32_get_unbiased_exp(float32 f) +{ + return ((f >> 23) & 0xFF) - 127; +} /*---------------------------------------------------------------------------- | The pattern for a default generated single-precision NaN. @@ -454,6 +465,12 @@ INLINE int float64_is_zero_or_denormal(float64 a) return (float64_val(a) & 0x7ff0000000000000LL) == 0; } +INLINE int float64_is_denormal(float64 a) +{ + return ((float64_val(a) & 0x7ff0000000000000LL) == 0) && + ((float64_val(a) & 0x000fffffffffffffLL) != 0); +} + INLINE float64 float64_set_sign(float64 a, int sign) { return make_float64((float64_val(a) & 0x7fffffffffffffffULL) @@ -472,6 +489,11 @@ INLINE float64 float64_set_sign(float64 a, int sign) *----------------------------------------------------------------------------*/ extern const float64 float64_default_nan; +INLINE int float64_get_unbiased_exp(float64 f) +{ + return ((f >> 52) & 0x7FF) - 1023; +} + /*---------------------------------------------------------------------------- | Software IEC/IEEE extended double-precision conversion routines. *----------------------------------------------------------------------------*/
This patch adds routines to the softfloat library that are useful for the PowerPC VSX implementation. The routines are, however, not specific to PowerPC and are approprriate for softfloat. The following routines are added: - float32_is_denormal() returns true if the 32-bit floating point number is denormalized. - float64_is_denormal() returns true if the 64-bit floating point number is denormalized. - float32_get_unbiased_exp() returns the unbiased exponent of a 32-bit floating point number. - float64_get_unbiased_exp() returns the unbiased exponent of a 64-bit floating point number. - float32_to_uint64() converts a 32-bit floating point number to an unsigned 64 bit number. Note that this patch is dependent a previously submitted patch that fixes the float64_to_uint64 conversion routine; see http://lists.nongnu.org/archive/html/qemu-devel/2013-10/msg02622.html for details. This contribution can be licensed under either the softfloat-2a or -2b license. Signed-off-by: Tom Musta <tommusta@gmail.com> --- fpu/softfloat.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ include/fpu/softfloat.h | 22 ++++++++++++++++++++++ 2 files changed, 67 insertions(+), 0 deletions(-)