This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: soft-fp: Support more precise "invalid" exceptions
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>, libc-alpha at sourceware dot org
- Date: Wed, 08 Oct 2014 20:24:28 -0400
- Subject: Re: soft-fp: Support more precise "invalid" exceptions
- Authentication-results: sourceware.org; auth=none
- References: <Pine dot LNX dot 4 dot 64 dot 1409190046310 dot 20342 at digraph dot polyomino dot org dot uk> <Pine dot LNX dot 4 dot 64 dot 1409190128300 dot 20342 at digraph dot polyomino dot org dot uk>
On 09/18/2014 09:29 PM, Joseph S. Myers wrote:
> Here is a corrected version of this patch with a bug in _FP_TO_INT's sNaN
> detection fixed.
Ignoring original patch and reviewing this one.
The changes in indentation make this patch annoying to review.
Your changes to _FP_CMP_CHECK_NAN are IMO too complicated
to follow now.
OK from me only if you simplify _FP_CMP_CHECK_NAN in *some* way to
make it easier to read, maintain and review.
> diff --git a/soft-fp/op-common.h b/soft-fp/op-common.h
> index af859e2..833658d 100644
> --- a/soft-fp/op-common.h
> +++ b/soft-fp/op-common.h
> @@ -87,7 +87,8 @@
> X##_c = FP_CLS_NAN; \
> /* Check for signaling NaN. */ \
> if (_FP_FRAC_SNANP (fs, X)) \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID \
> + | FP_EX_INVALID_SNAN); \
OK.
> } \
> break; \
> } \
> @@ -123,14 +124,14 @@
>
> /* Check for a semi-raw value being a signaling NaN and raise the
> invalid exception if so. */
> -#define _FP_CHECK_SIGNAN_SEMIRAW(fs, wc, X) \
> - do \
> - { \
> - if (X##_e == _FP_EXPMAX_##fs \
> - && !_FP_FRAC_ZEROP_##wc (X) \
> - && _FP_FRAC_SNANP_SEMIRAW (fs, X)) \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> - } \
> +#define _FP_CHECK_SIGNAN_SEMIRAW(fs, wc, X) \
> + do \
> + { \
> + if (X##_e == _FP_EXPMAX_##fs \
> + && !_FP_FRAC_ZEROP_##wc (X) \
> + && _FP_FRAC_SNANP_SEMIRAW (fs, X)) \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_SNAN); \
OK.
> + } \
> while (0)
>
> /* Choose a NaN result from an operation on two semi-raw NaN
> @@ -741,7 +742,8 @@
> R##_s = _FP_NANSIGN_##fs; \
> _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> _FP_FRAC_SLL_##wc (R, _FP_WORKBITS); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID \
> + | FP_EX_INVALID_ISI); \
OK.
> } \
> else \
> { \
> @@ -893,7 +895,7 @@
> R##_s = _FP_NANSIGN_##fs; \
> R##_c = FP_CLS_NAN; \
> _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_IMZ); \
OK.
> break; \
> \
> default: \
> @@ -1057,7 +1059,7 @@
> _FP_FMA_T##_s = _FP_NANSIGN_##fs; \
> _FP_FMA_T##_c = FP_CLS_NAN; \
> _FP_FRAC_SET_##wc (_FP_FMA_T, _FP_NANFRAC_##fs); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_IMZ_FMA); \
OK.
> break; \
> \
> default: \
> @@ -1102,7 +1104,7 @@
> R##_s = _FP_NANSIGN_##fs; \
> R##_c = FP_CLS_NAN; \
> _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_ISI); \
OK.
> } \
> break; \
> \
> @@ -1176,7 +1178,10 @@
> R##_s = _FP_NANSIGN_##fs; \
> R##_c = FP_CLS_NAN; \
> _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID \
> + | (X##_c == FP_CLS_INF \
> + ? FP_EX_INVALID_IDI \
> + : FP_EX_INVALID_ZDZ)); \
OK.
> break; \
> \
> default: \
> @@ -1190,17 +1195,25 @@
> raise exceptions for signaling NaN operands, 2 to raise exceptions
> for all NaN operands. */
>
> -#define _FP_CMP_CHECK_NAN(fs, wc, X, Y, ex) \
> - do \
> - { \
> - if (ex) \
> - { \
> - if ((ex) == 2 \
> - || _FP_ISSIGNAN (fs, wc, X) \
> - || _FP_ISSIGNAN (fs, wc, Y)) \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> - } \
> - } \
> +#define _FP_CMP_CHECK_NAN(fs, wc, X, Y, ex) \
> + do \
> + { \
> + if (ex) \
> + { \
> + if (FP_EX_INVALID_SNAN || FP_EX_INVALID_VC) \
> + { \
> + if ((ex) == 2) \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_VC); \
> + if (_FP_ISSIGNAN (fs, wc, X) \
> + || _FP_ISSIGNAN (fs, wc, Y)) \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_SNAN); \
> + } \
> + else if ((ex) == 2 \
> + || _FP_ISSIGNAN (fs, wc, X) \
> + || _FP_ISSIGNAN (fs, wc, Y)) \
> + FP_SET_EXCEPTION (FP_EX_INVALID); \
> + } \
This is no longer OK for me. This is really hard to follow now.
This should really be broken out logically into a series of checks
that we leave up to the compiler to reorganize. Unless you can show
that such a source setup produces drastically worse code.
> + } \
> while (0)
>
> /* Main differential comparison routine. The inputs should be raw not
> @@ -1285,58 +1298,58 @@
>
> /* Main square root routine. The input value should be cooked. */
>
> -#define _FP_SQRT(fs, wc, R, X) \
> - do \
> - { \
> - _FP_FRAC_DECL_##wc (_FP_SQRT_T); \
> - _FP_FRAC_DECL_##wc (_FP_SQRT_S); \
> - _FP_W_TYPE _FP_SQRT_q; \
> - switch (X##_c) \
> - { \
> - case FP_CLS_NAN: \
> - _FP_FRAC_COPY_##wc (R, X); \
> - R##_s = X##_s; \
> - R##_c = FP_CLS_NAN; \
> - break; \
> - case FP_CLS_INF: \
> - if (X##_s) \
> - { \
> - R##_s = _FP_NANSIGN_##fs; \
> - R##_c = FP_CLS_NAN; /* NAN */ \
> - _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> - } \
> - else \
> - { \
> - R##_s = 0; \
> - R##_c = FP_CLS_INF; /* sqrt(+inf) = +inf */ \
> - } \
> - break; \
> - case FP_CLS_ZERO: \
> - R##_s = X##_s; \
> - R##_c = FP_CLS_ZERO; /* sqrt(+-0) = +-0 */ \
> - break; \
> - case FP_CLS_NORMAL: \
> - R##_s = 0; \
> - if (X##_s) \
> - { \
> - R##_c = FP_CLS_NAN; /* NAN */ \
> - R##_s = _FP_NANSIGN_##fs; \
> - _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> - break; \
> - } \
> - R##_c = FP_CLS_NORMAL; \
> - if (X##_e & 1) \
> - _FP_FRAC_SLL_##wc (X, 1); \
> - R##_e = X##_e >> 1; \
> - _FP_FRAC_SET_##wc (_FP_SQRT_S, _FP_ZEROFRAC_##wc); \
> - _FP_FRAC_SET_##wc (R, _FP_ZEROFRAC_##wc); \
> - _FP_SQRT_q = _FP_OVERFLOW_##fs >> 1; \
> - _FP_SQRT_MEAT_##wc (R, _FP_SQRT_S, _FP_SQRT_T, X, \
> - _FP_SQRT_q); \
> - } \
> - } \
The change in indentation makes this hard to review.
Please try to avoid it.
> +#define _FP_SQRT(fs, wc, R, X) \
> + do \
> + { \
> + _FP_FRAC_DECL_##wc (_FP_SQRT_T); \
> + _FP_FRAC_DECL_##wc (_FP_SQRT_S); \
> + _FP_W_TYPE _FP_SQRT_q; \
> + switch (X##_c) \
> + { \
> + case FP_CLS_NAN: \
> + _FP_FRAC_COPY_##wc (R, X); \
> + R##_s = X##_s; \
> + R##_c = FP_CLS_NAN; \
> + break; \
> + case FP_CLS_INF: \
> + if (X##_s) \
> + { \
> + R##_s = _FP_NANSIGN_##fs; \
> + R##_c = FP_CLS_NAN; /* NAN */ \
> + _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_SQRT); \
> + } \
> + else \
> + { \
> + R##_s = 0; \
> + R##_c = FP_CLS_INF; /* sqrt(+inf) = +inf */ \
> + } \
> + break; \
> + case FP_CLS_ZERO: \
> + R##_s = X##_s; \
> + R##_c = FP_CLS_ZERO; /* sqrt(+-0) = +-0 */ \
> + break; \
> + case FP_CLS_NORMAL: \
> + R##_s = 0; \
> + if (X##_s) \
> + { \
> + R##_c = FP_CLS_NAN; /* NAN */ \
> + R##_s = _FP_NANSIGN_##fs; \
> + _FP_FRAC_SET_##wc (R, _FP_NANFRAC_##fs); \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_SQRT); \
OK. Change is here for FP_EX_INVALID_SQRT.
> + break; \
> + } \
> + R##_c = FP_CLS_NORMAL; \
> + if (X##_e & 1) \
> + _FP_FRAC_SLL_##wc (X, 1); \
> + R##_e = X##_e >> 1; \
> + _FP_FRAC_SET_##wc (_FP_SQRT_S, _FP_ZEROFRAC_##wc); \
> + _FP_FRAC_SET_##wc (R, _FP_ZEROFRAC_##wc); \
> + _FP_SQRT_q = _FP_OVERFLOW_##fs >> 1; \
> + _FP_SQRT_MEAT_##wc (R, _FP_SQRT_S, _FP_SQRT_T, X, \
> + _FP_SQRT_q); \
> + } \
> + } \
OK.
> while (0)
>
> /* Convert from FP to integer. Input is raw. */
> @@ -1399,12 +1412,17 @@
> }) \
> : 0); \
> if (!_FP_FRAC_ZEROP_##wc (X)) \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_CVI); \
> else if (_FP_TO_INT_inexact) \
> FP_SET_EXCEPTION (FP_EX_INEXACT); \
> } \
> else \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID \
> + | FP_EX_INVALID_CVI \
> + | ((FP_EX_INVALID_SNAN \
> + && _FP_ISSIGNAN (fs, wc, X)) \
> + ? FP_EX_INVALID_SNAN \
> + : 0)); \
OK. This has the correction to raise the FP_EX_INVALID_SNAN correctly.
> } \
> else \
> { \
> @@ -1563,7 +1581,8 @@
> if (!_FP_FRAC_ZEROP_##swc (S)) \
> { \
> if (_FP_FRAC_SNANP (sfs, S)) \
> - FP_SET_EXCEPTION (FP_EX_INVALID); \
> + FP_SET_EXCEPTION (FP_EX_INVALID \
> + | FP_EX_INVALID_SNAN); \
OK.
> _FP_FRAC_SLL_##dwc (D, (_FP_FRACBITS_##dfs \
> - _FP_FRACBITS_##sfs)); \
> _FP_SETQNAN (dfs, dwc, D); \
> diff --git a/soft-fp/soft-fp.h b/soft-fp/soft-fp.h
> index 5fb7358..3ced6ca 100644
> --- a/soft-fp/soft-fp.h
> +++ b/soft-fp/soft-fp.h
> @@ -83,6 +83,44 @@
> # define FP_EX_DENORM 0
> #endif
>
> +/* Sub-exceptions of "invalid". */
> +/* Signaling NaN operand. */
> +#ifndef FP_EX_INVALID_SNAN
> +# define FP_EX_INVALID_SNAN 0
> +#endif
> +/* Inf * 0. */
> +#ifndef FP_EX_INVALID_IMZ
> +# define FP_EX_INVALID_IMZ 0
> +#endif
> +/* fma (Inf, 0, c). */
> +#ifndef FP_EX_INVALID_IMZ_FMA
> +# define FP_EX_INVALID_IMZ_FMA 0
> +#endif
> +/* Inf - Inf. */
> +#ifndef FP_EX_INVALID_ISI
> +# define FP_EX_INVALID_ISI 0
> +#endif
> +/* 0 / 0. */
> +#ifndef FP_EX_INVALID_ZDZ
> +# define FP_EX_INVALID_ZDZ 0
> +#endif
> +/* Inf / Inf. */
> +#ifndef FP_EX_INVALID_IDI
> +# define FP_EX_INVALID_IDI 0
> +#endif
> +/* sqrt (negative). */
> +#ifndef FP_EX_INVALID_SQRT
> +# define FP_EX_INVALID_SQRT 0
> +#endif
> +/* Invalid conversion to integer. */
> +#ifndef FP_EX_INVALID_CVI
> +# define FP_EX_INVALID_CVI 0
> +#endif
> +/* Invalid comparison. */
> +#ifndef FP_EX_INVALID_VC
> +# define FP_EX_INVALID_VC 0
> +#endif
> +
OK.
> /* _FP_STRUCT_LAYOUT may be defined as an attribute to determine the
> struct layout variant used for structures where bit-fields are used
> to access specific parts of binary floating-point numbers. This is
>
Cheers,
Carlos.