On 14 August 2016 at 01:33, Andrew Dutcher <and...@andrewdutcher.com> wrote:
> The macro require_valid_floatx80(value, status) will check for malformed
> extended precision encodings, and if one is found, generate an
> invalid-operation exception and return NaN. This check has been added to
> the beginning of the basic 80-bit float arithmetic functions.
Hi; thanks for this patch. I think it's in general good,
but you haven't applied it to enough of the floatx80 functions.
We also need to check:
* conversions from floatx80 to another float format
(floatx80_to_float32/64/128)
* conversions from floatx80 to integer
(various floatx80_to_* functions)
* comparisons
* floatx80_scalbn (used in the FSCALE insn)
[basically we need to cover everything that the intel manual
calls an "arithmetic instruction".]
I also have a few style issues which I'll remark on inline below.
> Version 2: fix code style
This sort of v1-to-v2 changelog should go below the '---' line,
so it doesn't go in the final commit message in git.
> Signed-off-by: Andrew Dutcher <and...@andrewdutcher.com>
> ---
> fpu/softfloat-specialize.h | 12 ++++++++++++
> fpu/softfloat.c | 11 +++++++++++
> include/fpu/softfloat.h | 13 +++++++++++++
> 3 files changed, 36 insertions(+)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 43d0890..0e6ec25 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -203,6 +203,18 @@ void float_raise(int8_t flags, float_status *status)
> }
>
>
> /*----------------------------------------------------------------------------
> +| Asserts that the given value must be a valid floatx80 encoding. If The
> given
> +| value is a pseudo-NaN, pseudo-infiinty, or un-normal, raise an invalid
> +| operation exception and cause the parent function to return NaN.
> +*----------------------------------------------------------------------------*/
> +
> +#define require_valid_floatx80(a, status) \
> + if (floatx80_invalid_encoding((a))) { \
> + float_raise(float_flag_invalid, (status)); \
> + return floatx80_default_nan((status)); \
> + }
Macros that incorporate hidden flow control tend to be bad for
readability, and the softfloat code is already dangerously
close to unreadable. I think it would be better to just expand
this macro out in the places where you're using it: saying
if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
float_raise(float_flag_invalid, status);
return floatx80_default_nan(status);
}
is a few more lines but I think clearer.
> +
> +/*----------------------------------------------------------------------------
> | Internal canonical NaN format.
>
> *----------------------------------------------------------------------------*/
> typedef struct {
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 9b1eccf..a921e5e 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -5279,6 +5279,8 @@ floatx80 floatx80_add(floatx80 a, floatx80 b,
> float_status *status)
> {
> flag aSign, bSign;
>
> + require_valid_floatx80(a, status);
> + require_valid_floatx80(b, status);
> aSign = extractFloatx80Sign( a );
> bSign = extractFloatx80Sign( b );
> if ( aSign == bSign ) {
> @@ -5300,6 +5302,8 @@ floatx80 floatx80_sub(floatx80 a, floatx80 b,
> float_status *status)
> {
> flag aSign, bSign;
>
> + require_valid_floatx80(a, status);
> + require_valid_floatx80(b, status);
> aSign = extractFloatx80Sign( a );
> bSign = extractFloatx80Sign( b );
> if ( aSign == bSign ) {
> @@ -5323,6 +5327,8 @@ floatx80 floatx80_mul(floatx80 a, floatx80 b,
> float_status *status)
> int32_t aExp, bExp, zExp;
> uint64_t aSig, bSig, zSig0, zSig1;
>
> + require_valid_floatx80(a, status);
> + require_valid_floatx80(b, status);
> aSig = extractFloatx80Frac( a );
> aExp = extractFloatx80Exp( a );
> aSign = extractFloatx80Sign( a );
> @@ -5380,6 +5386,8 @@ floatx80 floatx80_div(floatx80 a, floatx80 b,
> float_status *status)
> uint64_t aSig, bSig, zSig0, zSig1;
> uint64_t rem0, rem1, rem2, term0, term1, term2;
>
> + require_valid_floatx80(a, status);
> + require_valid_floatx80(b, status);
> aSig = extractFloatx80Frac( a );
> aExp = extractFloatx80Exp( a );
> aSign = extractFloatx80Sign( a );
> @@ -5461,6 +5469,8 @@ floatx80 floatx80_rem(floatx80 a, floatx80 b,
> float_status *status)
> uint64_t aSig0, aSig1, bSig;
> uint64_t q, term0, term1, alternateASig0, alternateASig1;
>
> + require_valid_floatx80(a, status);
> + require_valid_floatx80(b, status);
> aSig0 = extractFloatx80Frac( a );
> aExp = extractFloatx80Exp( a );
> aSign = extractFloatx80Sign( a );
> @@ -5556,6 +5566,7 @@ floatx80 floatx80_sqrt(floatx80 a, float_status *status)
> uint64_t aSig0, aSig1, zSig0, zSig1, doubleZSig0;
> uint64_t rem0, rem1, rem2, rem3, term0, term1, term2, term3;
>
> + require_valid_floatx80(a, status);
> aSig0 = extractFloatx80Frac( a );
> aExp = extractFloatx80Exp( a );
> aSign = extractFloatx80Sign( a );
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index 0e57ee5..569730f 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -671,6 +671,19 @@ static inline int floatx80_is_any_nan(floatx80 a)
> floatx80 floatx80_default_nan(float_status *status);
I would move this function up to just below
floatx80_is_any_nan() so all the "return some property
of the floatx80 encoding" functions are together.
>
>
> /*----------------------------------------------------------------------------
> +| Return whether the given value is an invalid floatx80 encoding.
> +| Invalid floatx80 encodings arise when the integer bit is not set, but
> +| the exponent is not zero. The only times the integer bit is permitted to
> +| be zero is in subnormal numbers and the value zero.
We could say here also
"This includes what the Intel software developer's manual calls
pseudo-NaNs, pseudo-infinities and un-normal numbers. It does not
include pseudo-denormals, which must still be correctly handled
as inputs even if they are never generated as outputs."
> +*----------------------------------------------------------------------------*/
> +
> +static inline bool floatx80_invalid_encoding(floatx80 a)
> +{
> + return !(a.low & 0x8000000000000000) && !!(a.high & 0x7FFF);
The '!!' isn't necessary, because && is a boolean operator.
If you want to be concise, you can just omit it. If you
want to be clear you can use "(a.high & 0x7fff) != 0".
"(1 << 63)" is easier to check against the spec than 0x8<lots of 0s>.
> +}
> +
> +
Stray extra blank line.
> +/*----------------------------------------------------------------------------
> | Software IEC/IEEE quadruple-precision conversion routines.
>
> *----------------------------------------------------------------------------*/
> int32_t float128_to_int32(float128, float_status *status);
> --
> 2.7.4
thanks
-- PMM
