code review 6500121: math: Faster Tanh (issue 6500121)

[cldo...@gmail.com]

Reviewers: rsc, golang-dev_googlegroups.com,

Message:
Hello r...@golang.org, golan...@googlegroups.com (cc:
golan...@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
math: Faster Tanh

From 159 to 49.4 ns/op; slightly more accurate.

Please review this at http://codereview.appspot.com/6500121/

Affected files:
M src/pkg/math/tanh.go


Index: src/pkg/math/tanh.go
===================================================================
--- a/src/pkg/math/tanh.go
+++ b/src/pkg/math/tanh.go
@@ -1,15 +1,67 @@
-// Copyright 2009 The Go Authors. All rights reserved.
+// Copyright 2012 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package math

-/*
- Floating-point hyperbolic tangent.
+// The original C code, the long comment, and the constants
+// below were from http://netlib.sandia.gov/cephes/cmath/sin.c,
+// available from http://www.netlib.org/cephes/cmath.tgz.
+// The go code is a simplified version of the original C.
+// tanh.c
+//
+// Hyperbolic tangent
+//
+// SYNOPSIS:
+//
+// double x, y, tanh();
+//
+// y = tanh( x );
+//
+// DESCRIPTION:
+//
+// Returns hyperbolic tangent of argument in the range MINLOG to MAXLOG.
+//
+// A rational function is used for |x| < 0.625. The form
+// x + x**3 P(x)/Q(x) of Cody & Waite is employed.
+// Otherwise,
+// tanh(x) = sinh(x)/cosh(x) = 1 - 2/(exp(2x) + 1).
+//
+// ACCURACY:
+//
+// Relative error:
+// arithmetic domain # trials peak rms
+// IEEE -2,2 30000 2.5e-16 5.8e-17
+//
+// Cephes Math Library Release 2.8: June, 2000
+// Copyright 1984, 1987, 1989, 1992, 2000 by Stephen L. Moshier
+//
+// The readme file at http://netlib.sandia.gov/cephes/ says:
+// Some software in this archive may be from the book _Methods and
+// Programs for Mathematical Functions_ (Prentice-Hall or Simon & Schuster
+// International, 1989) or from the Cephes Mathematical Library, a
+// commercial product. In either event, it is copyrighted by the author.
+// What you see here may be used freely but it comes with no support or
+// guarantee.
+//
+// The two known misprints in the book are repaired here in the
+// source listings for the gamma function and the incomplete beta
+// integral.
+//
+// Stephen L. Moshier
+// mos...@na-net.ornl.gov
+//

- Sinh and Cosh are called except for large arguments, which
- would cause overflow improperly.
-*/
+var tanhP = [...]float64{
+ -9.64399179425052238628E-1,
+ -9.92877231001918586564E1,
+ -1.61468768441708447952E3,
+}
+var tanhQ = [...]float64{
+ 1.12811678491632931402E2,
+ 2.23548839060100448583E3,
+ 4.84406305325125486048E3,
+}

// Tanh computes the hyperbolic tangent of x.
//
@@ -18,15 +70,28 @@
// Tanh(±Inf) = ±1
// Tanh(NaN) = NaN
func Tanh(x float64) float64 {
- if x < 0 {
- x = -x
- if x > 21 {
- return -1
+ const (
+ MAXLOG = 8.8029691931113054295988e+01 // log(2**127)
+ MINLOG = -8.872283911167299960540e+01 // log(2**-128)
+ )
+ z := Abs(x)
+ switch {
+ case z == 0:
+ return x
+ case z < 0.625:
+ s := x * x
+ z = x +
x*s*((tanhP[0]*s+tanhP[1])*s+tanhP[2])/(((s+tanhQ[0])*s+tanhQ[1])*s+tanhQ[2])
+ case z <= 0.5*MAXLOG:
+ s := Exp(2 * z)
+ z = 1 - 2/(s+1)
+ if x < 0 {
+ z = -z
}
- return -Sinh(x) / Cosh(x)
+ case z > 0.5*MAXLOG:
+ z = 1
+ if x < 0 {
+ z = -z
+ }
}
- if x > 21 {
- return 1
- }
- return Sinh(x) / Cosh(x)
+ return z
}

[r...@golang.org]

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/

[Charlie Dorian]

I included the constant MINLOG because it is mentioned in the
documentation; it is not used in the code.

[Charlie Dorian]

The two comparisons to MAXLOG allowed me to avoid another case and a
call to IsNaN.

[Michael Jones]

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 );

--
Michael T. Jones | Chief Technology Advocate | m...@google.com | +1
650-335-5765

[da...@cheney.net]

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.

http://codereview.appspot.com/6500121/

[remyoud...@gmail.com]

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)

http://codereview.appspot.com/6500121/

[cldo...@gmail.com]

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.


https://codereview.appspot.com/6500121/

[cldo...@gmail.com]

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.

https://codereview.appspot.com/6500121/

[cldo...@gmail.com]

Hello r...@golang.org, golan...@googlegroups.com, m...@google.com,
da...@cheney.net, remyoud...@gmail.com (cc:
golan...@googlegroups.com),

Please take another look.


http://codereview.appspot.com/6500121/

[cldo...@gmail.com]

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.


https://codereview.appspot.com/6500121/

[cldo...@gmail.com]

I reverted to the original logic sequence -- it was clearer (and faster
by 2 ns).

> >> On Thu, Sep 13, 2012 at 11:13 PM, <mailto:r...@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:m...@google.com
| +1
> 650-335-5765



https://codereview.appspot.com/6500121/

[r...@golang.org]

LGTM


https://codereview.appspot.com/6500121/

[r...@golang.org]

*** 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 <r...@golang.org>


http://codereview.appspot.com/6500121/