[PATCH] Fix truncf for sNaN input

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix truncf for sNaN input

Fabian Schriever
Make line 47 in sf_trunc.c reachable. While converting the double
precision function trunc to the single precision version truncf an error
was introduced into the special case. This special case is meant to
catch both NaNs and infinities, however qNaNs and infinities work just
fine with the simple return of x (line 51). The only error occurs for
sNaNs where the same sNaN is returned and no invalid exception is
raised.
---
 newlib/libm/common/sf_trunc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/newlib/libm/common/sf_trunc.c b/newlib/libm/common/sf_trunc.c
index 74ea933ce..8eb0554d8 100644
--- a/newlib/libm/common/sf_trunc.c
+++ b/newlib/libm/common/sf_trunc.c
@@ -42,7 +42,7 @@
     }
   else
     {
-      if (exponent_less_127 == 255)
+      if (exponent_less_127 == 128)
         /* x is NaN or infinite. */
         return x + x;
 
--
2.24.1.windows.2


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Sourceware - newlib list mailing list
On Mar 11 10:58, Fabian Schriever wrote:

> Make line 47 in sf_trunc.c reachable. While converting the double
> precision function trunc to the single precision version truncf an error
> was introduced into the special case. This special case is meant to
> catch both NaNs and infinities, however qNaNs and infinities work just
> fine with the simple return of x (line 51). The only error occurs for
> sNaNs where the same sNaN is returned and no invalid exception is
> raised.
> ---
>  newlib/libm/common/sf_trunc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/newlib/libm/common/sf_trunc.c b/newlib/libm/common/sf_trunc.c
> index 74ea933ce..8eb0554d8 100644
> --- a/newlib/libm/common/sf_trunc.c
> +++ b/newlib/libm/common/sf_trunc.c
> @@ -42,7 +42,7 @@
>      }
>    else
>      {
> -      if (exponent_less_127 == 255)
> +      if (exponent_less_127 == 128)
>          /* x is NaN or infinite. */
>          return x + x;
>  
> --
> 2.24.1.windows.2
>
Pushed.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Sourceware - newlib list mailing list
In reply to this post by Fabian Schriever
Fabian Schriever <[hidden email]> writes:

> Make line 47 in sf_trunc.c reachable. While converting the double
> precision function trunc to the single precision version truncf an error
> was introduced into the special case. This special case is meant to
> catch both NaNs and infinities, however qNaNs and infinities work just
> fine with the simple return of x (line 51). The only error occurs for
> sNaNs where the same sNaN is returned and no invalid exception is
> raised.

I think this isn't right. According to the trunc/truncf man page:

       If x is integral, infinite, or NaN, x itself is returned.

I think this means that we should just return x in these cases, and not
raise an exception?

The same appears to be true for the other similar functions, including
floor, ceil, round, rint and nearbyint.

--
-keith

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Joseph Myers
On Wed, 11 Mar 2020, Keith Packard via Newlib wrote:

> I think this isn't right. According to the trunc/truncf man page:
>
>        If x is integral, infinite, or NaN, x itself is returned.
>
> I think this means that we should just return x in these cases, and not
> raise an exception?

That manpage is wrong.  The underlying IEEE operation should raise
"invalid" and return a qNaN for an sNaN operand (but never raises
"inexact").

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Sourceware - newlib list mailing list
Joseph Myers <[hidden email]> writes:

> That manpage is wrong.  The underlying IEEE operation should raise
> "invalid" and return a qNaN for an sNaN operand (but never raises
> "inexact").

Hrm. IEEE 754 says that you only get "invalid" if the NaN or infinite
operand cannot be represented in the destination format. As these
functions return the same type as their operand, NaN and inf values can
be represented in the return value.

Are you referring to some other standard?

Also, glibc works the way the manual says, and I'd really like newlib
and glibc to have the same general behavior, even if newlib isn't quite
as accurate as glibc for some operations.

--
-keith

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Joseph Myers
On Wed, 11 Mar 2020, Keith Packard via Newlib wrote:

> Joseph Myers <[hidden email]> writes:
>
> > That manpage is wrong.  The underlying IEEE operation should raise
> > "invalid" and return a qNaN for an sNaN operand (but never raises
> > "inexact").
>
> Hrm. IEEE 754 says that you only get "invalid" if the NaN or infinite
> operand cannot be represented in the destination format. As these
> functions return the same type as their operand, NaN and inf values can
> be represented in the return value.
>
> Are you referring to some other standard?

I'm referring to 6.2 Operations with NaNs, "Signaling NaNs shall be
reserved operands that signal the invalid operation exception (see 7.2)
for every general-computational and signaling-computational operation
except for the conversions described in 5.12.".

> Also, glibc works the way the manual says, and I'd really like newlib
> and glibc to have the same general behavior, even if newlib isn't quite
> as accurate as glibc for some operations.

glibc's libm-test-trunc.inc explicitly verifies that sNaN inputs to trunc
functions result in a qNaN output with "invalid" but not "inexact" raised.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Sourceware - newlib list mailing list
Joseph Myers <[hidden email]> writes:

> I'm referring to 6.2 Operations with NaNs, "Signaling NaNs shall be
> reserved operands that signal the invalid operation exception (see 7.2)
> for every general-computational and signaling-computational operation
> except for the conversions described in 5.12.".

I believe you are concerned with the exception behavior, where the
original code was wrong. Section 5.9 says that conversion functions
signal for sNaN input, while 6.2 says that they deliver a qNaN result.

I was concerned with what NaN values are delivered from a NaN
operand, given the POSIX specification which says that the operand
itself shall be delivered if it is a NaN.

I hadn't considered the exception behavior, nor had I realized that sNaN
operands cause a qNaN to be delivered.

I need to interpret the POSIX spec as saying that a NaN operand will
deliver *some* NaN, but not that it will deliver the NaN operand. The
functions cannot do this in the case of sNaN operands which must deliver
a qNaN according to the IEEE spec.

I'll update the newlib tests to verify that sNaN parameters deliver qNaN
results, and to allow any qNaN value for a qNaN operand, instead of
requiring that the functions return precisely the same qNaN value. Those
tests don't currently encode the signal behavior; I'll have to add
support for that at some point.

Thanks much for your clarifications and pointers to the relevant specs.

--
-keith

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Joseph Myers
On Mon, 16 Mar 2020, Keith Packard via Newlib wrote:

> I was concerned with what NaN values are delivered from a NaN
> operand, given the POSIX specification which says that the operand
> itself shall be delivered if it is a NaN.

I don't think POSIX specifications should be trusted for how functions
should behave with sNaN inputs.  Rather, prefer the specifications from TS
18661-1 (or the latest C2x draft which has TS 18661-1 integrated).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Sourceware - newlib list mailing list
Joseph Myers <[hidden email]> writes:

> I don't think POSIX specifications should be trusted for how functions
> should behave with sNaN inputs.  Rather, prefer the specifications from TS
> 18661-1 (or the latest C2x draft which has TS 18661-1 integrated).

From my reading of that, I should only check for signaling vs quiet NaN,
ensuring that values are the right kind of NaN, but not any specific
value of NaN.

I've adjusted the newlib tests like this:

  /* All signaling nans are the "same", as are all quiet nans */
  if (isnan(correct.value) && isnan(is)
      && (issignaling(correct.value) == issignaling(is)))
  {
        /* PASS */
  }

Does this seem correct?

--
-keith

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Fabian Schriever
In reply to this post by Joseph Myers
Am 17/03/2020 um 02:30 schrieb Joseph Myers:
> On Mon, 16 Mar 2020, Keith Packard via Newlib wrote:
>
>> I was concerned with what NaN values are delivered from a NaN
>> operand, given the POSIX specification which says that the operand
>> itself shall be delivered if it is a NaN.
> I don't think POSIX specifications should be trusted for how functions
> should behave with sNaN inputs.  Rather, prefer the specifications from TS
> 18661-1 (or the latest C2x draft which has TS 18661-1 integrated).
>
I agree that both the IEEE-754 and C standards should take precedence
over POSIX, nonetheless POSIX does contain a chapter on the treatment of
NaNs including signaling NaNs: §4.20 (I only have access to the POSIX
draft 3 from 2007: http://www.open-std.org/jtc1/sc22/open/n4217.pdf). It
contains the following:

On implementations that support the IEC60559: 1989 standard floating
point, functions with signaling NaN argument(s) shall be treated as if
the function were called with an argument that is a required domain
error and shall return a quiet NaN result, except where stated otherwise.
Note:    The function might never see the signaling NaN, since it might
trigger when the arguments are evaluated during the function call.
On implementations that support the IEC60559: 1989standard floating
point, for those functions that do not have a documented domain error,
the following shall apply:
     These functions shall fail if:
     Domain Error   Any argument is a signaling NaN.
     Either, the integer expression (math_errhandling & MATH_ERRNO) is
non-zero and errno shall be set to [EDOM], or the integer expression
(math_errhandling &MATH_ERREXCEPT) is non-zeroand the invalid
floating-point exception shall be raised.

For this change I only sought to have the single-precision function
perform the same as it's double counterpart for now. I have not checked
whether the POSIX requirements regarding errno are met.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Brian Inglis
On 2020-03-17 09:26, Fabian Schriever wrote:
> Am 17/03/2020 um 02:30 schrieb Joseph Myers:
>> On Mon, 16 Mar 2020, Keith Packard via Newlib wrote:
>>> I was concerned with what NaN values are delivered from a NaN
>>> operand, given the POSIX specification which says that the operand
>>> itself shall be delivered if it is a NaN.
>> I don't think POSIX specifications should be trusted for how functions
>> should behave with sNaN inputs.  Rather, prefer the specifications from TS
>> 18661-1 (or the latest C2x draft which has TS 18661-1 integrated).

> I agree that both the IEEE-754 and C standards should take precedence over
> POSIX, nonetheless POSIX does contain a chapter on the treatment of NaNs
> including signaling NaNs: §4.20 (I only have access to the POSIX draft 3
> from 2007: http://www.open-std.org/jtc1/sc22/open/n4217.pdf). It contains
> the following:

Latest Volume 1. "Base Definitions" Chapter 4. "General Concepts" 4.21
"Treatment of NaN Arguments for the Mathematical Functions" available at:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_21

[free registration allows continuing access and downloads with minimal spam]

Unchanged:

> "On implementations that support the IEC 60559:1989 standard floating point,
> functions with signaling NaN argument(s) shall be treated as if the function
> were called with an argument that is a required domain error and shall return
> a quiet NaN result, except where stated otherwise.
> Note:
>      The function might never see the signaling NaN, since it might trigger
> when the arguments are evaluated during the function call.
> On implementations that support the IEC 60559:1989 standard floating point,
> for those functions that do not have a documented domain error, the following
> shall apply:
>     These functions shall fail if:
>     Domain Error   Any argument is a signaling NaN.
>     Either, the integer expression (math_errhandling & MATH_ERRNO) is non-zero
> and errno shall be set to [EDOM], or the integer expression (math_errhandling
> & MATH_ERREXCEPT) is non-zero and the invalid floating-point exception shall
> be raised."

Section 4.20 "Treatment of Error Conditions for Mathematical Functions" may also
apply.

> For this change I only sought to have the single-precision function perform the
> same as it's double counterpart for now. I have not checked whether the POSIX
> requirements regarding errno are met.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Joseph Myers
In reply to this post by Sourceware - newlib list mailing list
On Mon, 16 Mar 2020, Keith Packard via Newlib wrote:

> Joseph Myers <[hidden email]> writes:
>
> > I don't think POSIX specifications should be trusted for how functions
> > should behave with sNaN inputs.  Rather, prefer the specifications from TS
> > 18661-1 (or the latest C2x draft which has TS 18661-1 integrated).
>
> From my reading of that, I should only check for signaling vs quiet NaN,
> ensuring that values are the right kind of NaN, but not any specific
> value of NaN.
>
> I've adjusted the newlib tests like this:
>
>   /* All signaling nans are the "same", as are all quiet nans */
>   if (isnan(correct.value) && isnan(is)
>       && (issignaling(correct.value) == issignaling(is)))
>   {
> /* PASS */
>   }
>
> Does this seem correct?

Yes.  Only a few functions such as fabs and copysign are expected to
preserve NaN payloads (and those functions also preserve whether the NaN
is signaling and never raise exceptions).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix truncf for sNaN input

Joseph Myers
In reply to this post by Fabian Schriever
On Tue, 17 Mar 2020, Fabian Schriever wrote:

> On implementations that support the IEC60559: 1989 standard floating point,
> functions with signaling NaN argument(s) shall be treated as if the function
> were called with an argument that is a required domain error and shall return
> a quiet NaN result, except where stated otherwise.

This fails to allow for functions such as fabs where the corresponding
IEEE operations pass through a signaling NaN rather than returning a quiet
NaN with "invalid" raised.

Furthermore, TS 18661-1 makes it implementation-defined whether there is a
domain error.  And in practice it's more convenient for implementations
not to treat it as a domain error (not to set errno) as that means they
can just e.g. add a NaN argument to itself rather than rather than
explicitly testing for a signaling NaN.

--
Joseph S. Myers
[hidden email]