Issue 6500121: code review 6500121: math: Faster Tanh (Closed)
|
Created:
13 years, 9 months ago by Charlie Dorian Modified:
13 years, 9 months ago Reviewers:
CC:
rsc, golang-dev, mtj1, dave_cheney.net, remyoudompheng Visibility:
Public. |
Descriptionmath: Faster Tanh
From 159 to 47.6 ns/op; slightly more accurate.
Patch Set 1 #Patch Set 2 : diff -r 0d32d96b4cc1 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 0d32d96b4cc1 https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 4 : diff -r 600708342ddd https://go.googlecode.com/hg/ #MessagesTotal messages: 14
Hello rsc@golang.org, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
LGTM Let me know about the const - just want to make sure one of the MAXLOGs shouldn't be MINLOG. http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go File src/pkg/math/tanh.go (right): http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode75 src/pkg/math/tanh.go:75: MINLOG = -8.872283911167299960540e+01 // log(2**-128) Unused?
Sign in to reply to this message.
I included the constant MINLOG because it is mentioned in the documentation; it is not used in the code. On Thu, Sep 13, 2012 at 11:13 PM, <rsc@golang.org> wrote: > LGTM > > Let me know about the const - just want to make sure one of the MAXLOGs > shouldn't be MINLOG. > > > > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go > File src/pkg/math/tanh.go (right): > > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode75 > src/pkg/math/tanh.go:75: MINLOG = -8.872283911167299960540e+01 // > log(2**-128) > Unused? > > http://codereview.appspot.com/6500121/
Sign in to reply to this message.
The two comparisons to MAXLOG allowed me to avoid another case and a call to IsNaN. On Thu, Sep 13, 2012 at 11:17 PM, Charlie Dorian <cldorian@gmail.com> wrote: > I included the constant MINLOG because it is mentioned in the > documentation; it is not used in the code. > > On Thu, Sep 13, 2012 at 11:13 PM, <rsc@golang.org> wrote: >> LGTM >> >> Let me know about the const - just want to make sure one of the MAXLOGs >> shouldn't be MINLOG. >> >> >> >> http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go >> File src/pkg/math/tanh.go (right): >> >> http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode75 >> src/pkg/math/tanh.go:75: MINLOG = -8.872283911167299960540e+01 // >> log(2**-128) >> Unused? >> >> http://codereview.appspot.com/6500121/
Sign in to reply to this message.
Original looked like this:
z = fabs(x);
if( z > 0.5 * MAXLOG )
{
if( x > 0 )
return( 1.0 );
else
return( -1.0 );
}
if( z >= 0.625 )
{
s = exp(2.0*z);
z = 1.0 - 2.0/(s + 1.0);
if( x < 0 )
z = -z;
}
else
{
if( x == 0.0 )
return(x);
s = x * x;
z = polevl( s, P, 2 )/p1evl(s, Q, 3);
z = x * s * z;
z = x + z;
}
return( z );
On Thu, Sep 13, 2012 at 8:19 PM, Charlie Dorian <cldorian@gmail.com> wrote:
> The two comparisons to MAXLOG allowed me to avoid another case and a
> call to IsNaN.
>
> On Thu, Sep 13, 2012 at 11:17 PM, Charlie Dorian <cldorian@gmail.com> wrote:
>> I included the constant MINLOG because it is mentioned in the
>> documentation; it is not used in the code.
>>
>> On Thu, Sep 13, 2012 at 11:13 PM, <rsc@golang.org> wrote:
>>> LGTM
>>>
>>> Let me know about the const - just want to make sure one of the MAXLOGs
>>> shouldn't be MINLOG.
>>>
>>>
>>>
>>> http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go
>>> File src/pkg/math/tanh.go (right):
>>>
>>>
http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode75
>>> src/pkg/math/tanh.go:75: MINLOG = -8.872283911167299960540e+01 //
>>> log(2**-128)
>>> Unused?
>>>
>>> http://codereview.appspot.com/6500121/
--
Michael T. Jones | Chief Technology Advocate | mtj@google.com | +1
650-335-5765
Sign in to reply to this message.
http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go File src/pkg/math/tanh.go (right): http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode1 src/pkg/math/tanh.go:1: // Copyright 2012 The Go Authors. All rights reserved. We traditionally don't update the copyright date of existing files.
Sign in to reply to this message.
http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go File src/pkg/math/tanh.go (right): http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode55 src/pkg/math/tanh.go:55: var tanhP = [...]float64{ Is it faster/clearer to have these as constants instead of static variables? http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode83 src/pkg/math/tanh.go:83: z = x + x*s*((tanhP[0]*s+tanhP[1])*s+tanhP[2])/(((s+tanhQ[0])*s+tanhQ[1])*s+tanhQ[2]) x + x * s * ((P0 * s + P1) * s + P2) / ((Q0 * s + Q1) * s + Q2) (and I would reverse the indexing so that P(x) = P0 + P1·x² + P2·x⁴, Q(x) = Q0 + Q1·x² + Q2·x⁴ but that's a personal preference)
Sign in to reply to this message.
On 2012/09/14 09:04:18, remyoudompheng wrote: > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go > File src/pkg/math/tanh.go (right): > > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode55 > src/pkg/math/tanh.go:55: var tanhP = [...]float64{ > Is it faster/clearer to have these as constants instead of static variables? > I looked at this in November 2011. If you review issues 2446 and 2447, you will see the reasons for my choice of static variables rather than constants. If you think the floating-point code generation has improved since then, I'll be glad to re-do the tests. > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode83 > src/pkg/math/tanh.go:83: z = x + > x*s*((tanhP[0]*s+tanhP[1])*s+tanhP[2])/(((s+tanhQ[0])*s+tanhQ[1])*s+tanhQ[2]) > x + x * s * ((P0 * s + P1) * s + P2) / ((Q0 * s + Q1) * s + Q2) > > (and I would reverse the indexing so that P(x) = P0 + P1·x² + P2·x⁴, Q(x) = Q0 + > Q1·x² + Q2·x⁴ but that's a personal preference) I tend to place the highest exponent on the left (A·x² + B·x + C), but perhaps that's a relict of algebra teaching 50 years ago.
Sign in to reply to this message.
On 2012/09/14 06:33:39, dfc wrote: > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go > File src/pkg/math/tanh.go (right): > > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode1 > src/pkg/math/tanh.go:1: // Copyright 2012 The Go Authors. All rights reserved. > We traditionally don't update the copyright date of existing files. Thanks; I'll change it back. I wasn't sure of the tradition.
Sign in to reply to this message.
Hello rsc@golang.org, golang-dev@googlegroups.com, mtj@google.com, dave@cheney.net, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/09/14 03:13:40, rsc wrote: > LGTM > > Let me know about the const - just want to make sure one of the MAXLOGs > shouldn't be MINLOG. It is confusing, so I changed the code to put MAXLOG and MINLOG in the comments. Only MAXLOG remains in the function body. > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go > File src/pkg/math/tanh.go (right): > > http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode75 > src/pkg/math/tanh.go:75: MINLOG = -8.872283911167299960540e+01 // log(2**-128) > Unused? Removed from function body.
Sign in to reply to this message.
I reverted to the original logic sequence -- it was clearer (and faster by 2
ns).
On 2012/09/14 03:34:40, mtj1 wrote:
> Original looked like this:
>
> z = fabs(x);
> if( z > 0.5 * MAXLOG )
> {
> if( x > 0 )
> return( 1.0 );
> else
> return( -1.0 );
> }
> if( z >= 0.625 )
> {
> s = exp(2.0*z);
> z = 1.0 - 2.0/(s + 1.0);
> if( x < 0 )
> z = -z;
> }
> else
> {
> if( x == 0.0 )
> return(x);
> s = x * x;
> z = polevl( s, P, 2 )/p1evl(s, Q, 3);
> z = x * s * z;
> z = x + z;
> }
> return( z );
>
> On Thu, Sep 13, 2012 at 8:19 PM, Charlie Dorian <mailto:cldorian@gmail.com>
wrote:
> > The two comparisons to MAXLOG allowed me to avoid another case and a
> > call to IsNaN.
> >
> > On Thu, Sep 13, 2012 at 11:17 PM, Charlie Dorian <mailto:cldorian@gmail.com>
wrote:
> >> I included the constant MINLOG because it is mentioned in the
> >> documentation; it is not used in the code.
> >>
> >> On Thu, Sep 13, 2012 at 11:13 PM, <mailto:rsc@golang.org> wrote:
> >>> LGTM
> >>>
> >>> Let me know about the const - just want to make sure one of the MAXLOGs
> >>> shouldn't be MINLOG.
> >>>
> >>>
> >>>
> >>> http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go
> >>> File src/pkg/math/tanh.go (right):
> >>>
> >>>
> http://codereview.appspot.com/6500121/diff/5001/src/pkg/math/tanh.go#newcode75
> >>> src/pkg/math/tanh.go:75: MINLOG = -8.872283911167299960540e+01 //
> >>> log(2**-128)
> >>> Unused?
> >>>
> >>> http://codereview.appspot.com/6500121/
>
>
>
> --
> Michael T. Jones | Chief Technology Advocate | mailto:mtj@google.com | +1
> 650-335-5765
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=595850cc051e *** math: Faster Tanh From 159 to 47.6 ns/op; slightly more accurate. R=rsc, golang-dev, mtj, dave, remyoudompheng CC=golang-dev http://codereview.appspot.com/6500121 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|
