Issue 488003005: Port fdlibm implementation for Math.sinh.

Issue 488003005: Port fdlibm implementation for Math.sinh. (Closed)

Created:
6 years, 3 months ago by Yang

Modified:
6 years, 3 months ago

Reviewers:
Raymond Toy

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Project:
v8

Visibility:
Public.

More Reviews

Description

Port fdlibm implementation for Math.sinh. R=rtoy@chromium.org BUG=v8:3493 LOG=N Committed: https://code.google.com/p/v8/source/detail?r=23512

Patch Set 4 : added TODO #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+94 lines, -20 lines)			Patch
M	src/math.js	View		2 chunks	+1 line, -9 lines	0 comments	Download
M	test/mjsunit/es6/math-hyperbolic.js	View	1 2 3	5 chunks	+40 lines, -8 lines	0 comments	Download
M	third_party/fdlibm/fdlibm.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/fdlibm/fdlibm.cc	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	third_party/fdlibm/fdlibm.js	View	1 2	1 chunk	+49 lines, -0 lines	0 comments	Download
M	tools/generate-runtime-tests.py	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Raymond Toy

Code looks good. Just a few questions. https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hyperbolic.js File test/mjsunit/es6/math-hyperbolic.js (right): https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hyperbolic.js#newcode162 test/mjsunit/es6/math-hyperbolic.js:162: assertEquals(-1.7976931348621744e308, Math.sinh(-710.4758600739439)); ...

6 years, 3 months ago (2014-08-27 16:22:02 UTC) #2

Yang

https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hyperbolic.js File test/mjsunit/es6/math-hyperbolic.js (right): https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hyperbolic.js#newcode162 test/mjsunit/es6/math-hyperbolic.js:162: assertEquals(-1.7976931348621744e308, Math.sinh(-710.4758600739439)); On 2014/08/27 16:22:02, Raymond Toy wrote: > ...

6 years, 3 months ago (2014-08-28 07:18:08 UTC) #3

Raymond Toy

lgtm for the code. For the tests, I disagree on running the tests with --no-fast-math ...

6 years, 3 months ago (2014-08-28 17:15:55 UTC) #4

Yang

On 2014/08/28 17:15:55, Raymond Toy wrote: > lgtm for the code. > > For the ...

6 years, 3 months ago (2014-08-29 09:19:23 UTC) #5

On 2014/08/28 17:15:55, Raymond Toy wrote:
> lgtm for the code.
> 
> For the tests, I disagree on running the tests with --no-fast-math since
that's
> not how V8 is normally run, right?
> 
> But I won't block this CL for that.
> 
>
https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hy...
> File test/mjsunit/es6/math-hyperbolic.js (right):
> 
>
https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hy...
> test/mjsunit/es6/math-hyperbolic.js:105: assertEqualsDelta(-74.203210577788,
> Math.sinh(-5), 1E-12);
> Are these two sinh tests needed now?
> 
> Perhaps you can replace them with the exact fdlibm value of 74.20321057778875.
> 
>
https://codereview.chromium.org/488003005/diff/40001/test/mjsunit/es6/math-hy...
> test/mjsunit/es6/math-hyperbolic.js:162: assertEquals(-1.7976931348621744e308,
> Math.sinh(-710.4758600739439));
> On 2014/08/28 07:18:08, Yang wrote:
> > On 2014/08/27 16:22:02, Raymond Toy wrote:
> > > Do the tests in lines 155-162 actually pass? They do not for me because
> > Math.exp
> > > is inaccurate for these values.
> > 
> > They do, with --no-fast-math we use the slow library implementation for exp.
> 
> If this is how you normally run these tests, then ok. Otherwise, I think this
is
> a bad idea since chrome doesn't normally use --no-fast-math, right?
> 
>
https://codereview.chromium.org/488003005/diff/40001/third_party/fdlibm/fdlib...
> File third_party/fdlibm/fdlibm.js (right):
> 
>
https://codereview.chromium.org/488003005/diff/40001/third_party/fdlibm/fdlib...
> third_party/fdlibm/fdlibm.js:733: const KSINH_OVERFLOW = kMath[52];
> On 2014/08/28 07:18:08, Yang wrote:
> > On 2014/08/27 16:22:02, Raymond Toy wrote:
> > > Why is KSINH_OVERFLOW an element of kMath, but TWO_M28 and LOG_MAXD are
not?
> > > 
> > > This is ok, but I'm curious why these constants are treated differently.
> > 
> > TWO_M28 and LOG_MAXD have empty lower halves, so the machine code emitted
for
> > those constants is good enough so that we don't need to load it from a
> constant
> > pool.
> 
> Ok.

I'll add a TODO to implement a new exp.

Yang

6 years, 3 months ago (2014-08-29 09:33:04 UTC) #6

Message was sent while issue was closed.

Committed patchset #4 (id:60001) manually as 23512 (presubmit successful).

Expand Messages | Collapse Messages