[PATCH] Don't do double divide in powf.

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

[PATCH] Don't do double divide in powf.

Jim Wilson-2
Similar to the previous powf patch, we have a double 0.0/0.0 divide being
used to generate a NaN, which on a single float only target pulls in divdf3.
wf_log.c calls nan instead.  I fixed powf to do the same.

With this patch, and a small testcase, I see

gamma02:2368$ ls -lt a.out*
-rwxrwxr-x 1 jimw jimw 32912 Dec 11 19:35 a.out
-rwxrwxr-x 1 jimw jimw 46992 Dec 11 18:54 a.out.unpatched
gamma02:2369$

So the patched code is about 70% of the size of the unpatched code.

This was tested with the testcase included below, and a newlib make check.

Jim

        newlib/
        * libm/math/wf_pow.c (powf): Call nan instead of double 0.0/0.0.
---
 newlib/libm/math/wf_pow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/newlib/libm/math/wf_pow.c b/newlib/libm/math/wf_pow.c
index be453558b..1e0447173 100644
--- a/newlib/libm/math/wf_pow.c
+++ b/newlib/libm/math/wf_pow.c
@@ -108,7 +108,7 @@
     if (_LIB_VERSION == _SVID_)
         exc.retval = 0.0;
     else
-        exc.retval = 0.0/0.0; /* X/Open allow NaN */
+        exc.retval = nan(""); /* X/Open allow NaN */
     if (_LIB_VERSION == _POSIX_)
         errno = EDOM;
     else if (!matherr(&exc)) {
--
2.14.1

Testcase:
#include <stdlib.h>
#include <math.h>

int
main (void)
{
  union { float f; int i; } u, v;
  v.f = __builtin_nan ("");

  float f = powf (-2.0, -2.2);
  u.f = f;
  if (u.i != v.i)
    abort ();

  float g = powf (-2.0, 3.3);
  u.f = g;
  if (u.i != v.i)
    abort ();

  return 0;
}
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't do double divide in powf.

Corinna Vinschen
On Dec 11 19:45, Jim Wilson wrote:

> Similar to the previous powf patch, we have a double 0.0/0.0 divide being
> used to generate a NaN, which on a single float only target pulls in divdf3.
> wf_log.c calls nan instead.  I fixed powf to do the same.
>
> With this patch, and a small testcase, I see
>
> gamma02:2368$ ls -lt a.out*
> -rwxrwxr-x 1 jimw jimw 32912 Dec 11 19:35 a.out
> -rwxrwxr-x 1 jimw jimw 46992 Dec 11 18:54 a.out.unpatched
> gamma02:2369$
>
> So the patched code is about 70% of the size of the unpatched code.
>
> This was tested with the testcase included below, and a newlib make check.
>
> Jim
>
> newlib/
> * libm/math/wf_pow.c (powf): Call nan instead of double 0.0/0.0.
> ---
>  newlib/libm/math/wf_pow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/newlib/libm/math/wf_pow.c b/newlib/libm/math/wf_pow.c
> index be453558b..1e0447173 100644
> --- a/newlib/libm/math/wf_pow.c
> +++ b/newlib/libm/math/wf_pow.c
> @@ -108,7 +108,7 @@
>      if (_LIB_VERSION == _SVID_)
>          exc.retval = 0.0;
>      else
> -        exc.retval = 0.0/0.0; /* X/Open allow NaN */
> +        exc.retval = nan(""); /* X/Open allow NaN */
As far as I can see, that still pulls in an unnecessary function call,
nan().

What about using NAN?  That's basically __builtin_nanf("") and usually
resolves without function call on any platform.


Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

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

Re: [PATCH] Don't do double divide in powf.

Craig Howland
On 12/12/2017 08:04 AM, Corinna Vinschen wrote:

> On Dec 11 19:45, Jim Wilson wrote:
>> Similar to the previous powf patch, we have a double 0.0/0.0 divide being
>> used to generate a NaN, which on a single float only target pulls in divdf3.
>> wf_log.c calls nan instead.  I fixed powf to do the same.
>>
>> With this patch, and a small testcase, I see
>>
>> gamma02:2368$ ls -lt a.out*
>> -rwxrwxr-x 1 jimw jimw 32912 Dec 11 19:35 a.out
>> -rwxrwxr-x 1 jimw jimw 46992 Dec 11 18:54 a.out.unpatched
>> gamma02:2369$
>>
>> So the patched code is about 70% of the size of the unpatched code.
>>
>> This was tested with the testcase included below, and a newlib make check.
>>
>> Jim
>>
>> newlib/
>> * libm/math/wf_pow.c (powf): Call nan instead of double 0.0/0.0.
>> ---
>>   newlib/libm/math/wf_pow.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/newlib/libm/math/wf_pow.c b/newlib/libm/math/wf_pow.c
>> index be453558b..1e0447173 100644
>> --- a/newlib/libm/math/wf_pow.c
>> +++ b/newlib/libm/math/wf_pow.c
>> @@ -108,7 +108,7 @@
>>      if (_LIB_VERSION == _SVID_)
>>          exc.retval = 0.0;
>>      else
>> -        exc.retval = 0.0/0.0; /* X/Open allow NaN */
>> +        exc.retval = nan(""); /* X/Open allow NaN */
> As far as I can see, that still pulls in an unnecessary function call,
> nan().
>
> What about using NAN?  That's basically __builtin_nanf("") and usually
> resolves without function call on any platform.
No to either of the above.  The 0/0 construct is to cause a floating point
"invalid" exception, which the functions do not do.  (The divide has to be done
at runtime to cause the exception.)  The double divide can be made float by
changing it to 0.0F/0.0F.
Craig
Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] Don't do double divide in powf.

Jon Beniston-3
>>> --- a/newlib/libm/math/wf_pow.c
>>> +++ b/newlib/libm/math/wf_pow.c
>>> @@ -108,7 +108,7 @@
>>>      if (_LIB_VERSION == _SVID_)
>>>          exc.retval = 0.0;
>>>      else
>>> -        exc.retval = 0.0/0.0; /* X/Open allow NaN */
>>> +        exc.retval = nan(""); /* X/Open allow NaN */
>> As far as I can see, that still pulls in an unnecessary function call,
>> nan().
>>
>> What about using NAN?  That's basically __builtin_nanf("") and usually
>> resolves without function call on any platform.
>No to either of the above.  The 0/0 construct is to cause a floating point "invalid"
>exception, which the functions do not do.  (The divide has to be done
>at runtime to cause the exception.)  The double divide
>can be made float by changing it to 0.0F/0.0F.

That doesn't always work though, particularly with s/w floating point libs.

Could it be abstracted to a target specific function/macro to raise an invalid exception / return a NaN?

There's quite a few other functions that use the technique.

Cheers,
Jon

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't do double divide in powf.

Craig Howland
On 12/12/2017 11:50 AM, Jon Beniston wrote:

>>>> --- a/newlib/libm/math/wf_pow.c
>>>> +++ b/newlib/libm/math/wf_pow.c
>>>> @@ -108,7 +108,7 @@
>>>>        if (_LIB_VERSION == _SVID_)
>>>>            exc.retval = 0.0;
>>>>        else
>>>> -        exc.retval = 0.0/0.0; /* X/Open allow NaN */
>>>> +        exc.retval = nan(""); /* X/Open allow NaN */
>>> As far as I can see, that still pulls in an unnecessary function call,
>>> nan().
>>>
>>> What about using NAN?  That's basically __builtin_nanf("") and usually
>>> resolves without function call on any platform.
>> No to either of the above.  The 0/0 construct is to cause a floating point "invalid"
>> exception, which the functions do not do.  (The divide has to be done
>> at runtime to cause the exception.)  The double divide
>> can be made float by changing it to 0.0F/0.0F.
> That doesn't always work though, particularly with s/w floating point libs.
>
> Could it be abstracted to a target specific function/macro to raise an invalid exception / return a NaN?
>
> There's quite a few other functions that use the technique.
      Well, if it does not work then either 1) the particular compiler and SW
lib in question are not complying with the C standard, or 2) the specific
exception is not supported.  Case 2 is fine because it is permitted.
      While it theoretically can be abstracted, in practice for Newlib it cannot
be because fenv.h support is lacking.  The beauty of things like 0.0/0.0 is that
they give the desired results in a target-independent manner.  Coding of fenv
functions (e.g. feraiseexcept()) either end up using the same tricks to remain
target independent or require target-specific coding (whether related to the HW
or a SW implementation).
      I think, however, that in general if an implementation fails to work as
you say sometimes happens that it won't actually matter. (Assuming you mean that
it does not generate the exception, as opposed to choosing to use double instead
of float for the divide in the case under consideration.)  That is, if the
exception is not generated due to a bug, it can be considered to be the same as
if it were not supported by design.
      If, however, you are saying that 0.0F/0.0F might actually result in a
double soft divide routine being called, then we're up against a bug that should
be addressed in some manner that does not break properly-working
implementations.  (This same general technique is used in lots of places, even
though the specific 0.0/0.0 looks to only be in maybe only 6 float functions.) 
One means of doing this would be to add partial fenv.h support, but this is far
beyond the scope of the patch under consideration.
      To put it in a different way:  even though Newlib does not presently have
direct fenv.h support, the math routines in general were written to properly
support floating-point exceptions.  We should not break that so that we will
more easily be able to add the fenv.h support in the future, nor break any
applications which have added it for themselves.
      In this case, the 0.0/0.0 is a real mistake in the source and the correct
fix is to make it 0.0F/0.0F.  If there are some buggy implementations that don't
deal with this properly, then they can be the subject of a future patch for them.
             Craig
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't do double divide in powf.

Jim Wilson-2
In reply to this post by Corinna Vinschen
On 12/12/2017 05:04 AM, Corinna Vinschen wrote:
> On Dec 11 19:45, Jim Wilson wrote:
>> -        exc.retval = 0.0/0.0; /* X/Open allow NaN */
>> +        exc.retval = nan(""); /* X/Open allow NaN */
>
> As far as I can see, that still pulls in an unnecessary function call,
> nan().
>
> What about using NAN?  That's basically __builtin_nanf("") and usually
> resolves without function call on any platform.

I don't see any place in the math dir that currently uses NAN like this,
but there are multiple places calling nan(""), so I did it that way.  I
don't care which way it is done.  But it looks like Craig Howland has a
legitimate objection, so I will try float 0.0/0.0 divide instead.

Jim
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Don't do double divide in powf.

Jim Wilson-2
In reply to this post by Craig Howland
On 12/12/2017 08:06 AM, Craig Howland wrote:
> No to either of the above.  The 0/0 construct is to cause a floating
> point "invalid" exception, which the functions do not do.  (The divide
> has to be done at runtime to cause the exception.)  The double divide
> can be made float by changing it to 0.0F/0.0F.

After double checking the ISO C standard Annex F, I have to agree with you.

However, it looks like there are other places that already make this
mistake.  wf_log.c for instance calls nan, and the ISO C Annex F says
this case should generate an invalid exception too.  There may also be
other places that are broken in the same way.  This is no excuse for me
to make it worse though, so I will post a new patch.

Jim
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Don't do double divide in powf.

Jim Wilson-2
In reply to this post by Jim Wilson-2
Updated patch to use 0.0f/0.0f to avoid the divdf3 call.

Tested same way as before, with a testcase that triggers the code and
make check.

OK?

        newlib/
        * libm/math/wf_pow.c (powf): Use 0.0f instead 0.0 in divide.
---
 newlib/libm/math/wf_pow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/newlib/libm/math/wf_pow.c b/newlib/libm/math/wf_pow.c
index be453558b..7f4c6b651 100644
--- a/newlib/libm/math/wf_pow.c
+++ b/newlib/libm/math/wf_pow.c
@@ -108,7 +108,9 @@
     if (_LIB_VERSION == _SVID_)
         exc.retval = 0.0;
     else
-        exc.retval = 0.0/0.0; /* X/Open allow NaN */
+        /* Use a float divide, to avoid a soft-float double
+   divide call on single-float only targets.  */
+        exc.retval = (0.0f/0.0f); /* X/Open allow NaN */
     if (_LIB_VERSION == _POSIX_)
         errno = EDOM;
     else if (!matherr(&exc)) {
--
2.14.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] Don't do double divide in powf.

Corinna Vinschen
On Dec 12 11:39, Jim Wilson wrote:
> Updated patch to use 0.0f/0.0f to avoid the divdf3 call.
>
> Tested same way as before, with a testcase that triggers the code and
> make check.
>
> OK?
>
> newlib/
> * libm/math/wf_pow.c (powf): Use 0.0f instead 0.0 in divide.

Pushed with fixed log message.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

signature.asc (836 bytes) Download Attachment