Implement trigonometric functions using a fdlibm port.

Implement trigonometric functions using a fdlibm port.

R=jochen@chromium.org, rtoy@chromium.org, svenpanne@chromium.org
BUG=v8:3006
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=22918

[Sven Panne, 2014-07-24 10:44:21]

https://codereview.chromium.org/411263004/diff/1/src/rempio2.h
File src/rempio2.h (right):

https://codereview.chromium.org/411263004/diff/1/src/rempio2.h#newcode27
src/rempio2.h:27: static void* constants() { return
const_cast<double*>(constants_); }
Casting away const here looks wrong: The array itself is in read-only memory,
and any write access will lead to a segfault. If the caller needs a non-const
pointer, the const_cast should be there (still dangerous) or it should copy the
contents into RW memory. In our case: After const_casting in bootstrapper.cc,
writing into kTrig on the JavaScript side leads to a segfault, which is
semi-nice, but it's OK if there is a warning comment.

Now that the const_cast in TrigonometricConstants has been removed, there is not
much need for a real class anymore:

   struct TrigonometricConstants {
     static const double constants[34];
   };

is enough, the size can be calculated via ARRAY_SIZE. I want C++11's uniform
initialization and std::array... :-/

[Yang, 2014-07-24 11:00:34]

https://codereview.chromium.org/411263004/diff/1/src/rempio2.h
File src/rempio2.h (right):

https://codereview.chromium.org/411263004/diff/1/src/rempio2.h#newcode27
src/rempio2.h:27: static void* constants() { return
const_cast<double*>(constants_); }
On 2014/07/24 10:44:21, Sven Panne wrote:
> Casting away const here looks wrong: The array itself is in read-only memory,
> and any write access will lead to a segfault. If the caller needs a non-const
> pointer, the const_cast should be there (still dangerous) or it should copy
the
> contents into RW memory. In our case: After const_casting in bootstrapper.cc,
> writing into kTrig on the JavaScript side leads to a segfault, which is
> semi-nice, but it's OK if there is a warning comment.
> 
> Now that the const_cast in TrigonometricConstants has been removed, there is
not
> much need for a real class anymore:
> 
>    struct TrigonometricConstants {
>      static const double constants[34];
>    };
> 
> is enough, the size can be calculated via ARRAY_SIZE. I want C++11's uniform
> initialization and std::array... :-/

Done.

[Sven Panne, 2014-07-24 11:49:44]

LGTM (the math part is basically rubber-stamped)

[Raymond Toy, 2014-07-24 16:51:12]

I'm not a reviewer, but I'm making some comments anyway.

I think the code is correct after comparing it the reference from
www/~rtoy/V8/kernel-trig.js, which I carefully compared against the fdlibm code
from which it was derived.

My comments are just nits to improve readability.

https://codereview.chromium.org/411263004/diff/20001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode196
src/math.js:196: var z = X - kTrig[1];
Readability would be improved if the kTrig values used here were actual
constants. Now it's pretty hard to know that kTrig[2] was pio2_1t. (That's not
really clear either, but at least it hints that the value is probably related to
pi/2, which it is.)

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode262
src/math.js:262: // Sine for [-pi/4, pi/4], pi/4 ~ 0.7854
I think it would be beneficial to add the original fdlibm comment on how this
routine works.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode268
src/math.js:268: return (X0 - ((z * (0.5 * X1 - v * r) - X1) - v * kTrig[7]))
SIGN;
The original kernel_sin function had a slight optimization where if x1 were
known to be zero, some of the computations could be removed. It doesn't affect
the result to do the extra computations, but there are extra computations.

Is it not needed?

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode269
src/math.js:269: endmacro
I think readability would be enhanced if you had a separate array for the
coefficients here instead of using kTrig[7-12].  Same goes for the cosine and
tangent coefficients below.

Is it really necessary to use just one array?

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode275
src/math.js:275: var r = z * (kTrig[13] + z * (kTrig[14] + z * (kTrig[15] +
Can we use a separate array for kTrig values here to indicate that these are the
coefficients of the cos polynomial approximation?

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode281
src/math.js:281: if (ix > 0x3fe90000) {
I think it's really useful to include the fdlibm comment about how this function
works. Without it, it's really impossible to understand why there's a test here
for 0x3fe90000 and what the code is trying to do with that.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode291
src/math.js:291: // Tangent for [-pi/4, pi/4], pi/4 ~ 0.7854
Include original fdlibm comment. Without that it's really hard to figure out
that even though this is called KernelTan, it can return either tan(x) or cot(x)
depending on the value of returnTan.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode331
src/math.js:331: var r = kTrig[20] + w * (kTrig[22] + w * (kTrig[24] +
Can we use a separate array for the coefficients instead of kTrig?

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode355
src/math.js:355: function MathSinSlow(x) {
I think MathSinAccurate is a better name. Or maybe MathSinWithPIReduction.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode365
src/math.js:365: function MathCosSlow(x) {
Similar comment as MathSinSlow.

https://codereview.chromium.org/411263004/diff/20001/src/rempio2.cc
File src/rempio2.cc (right):

https://codereview.chromium.org/411263004/diff/20001/src/rempio2.cc#newcode61
src/rempio2.cc:61: };
I think readability would be vastly improved if you didn't smash all of these
constants into one giant array.  At the very least, the S1-S6, C1-C6 and T0-T12
values should be in their own arrays.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js
File test/mjsunit/sin-cos.js (right):

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:194: // Tests for Math.sin for |x| < pi/4
I think you should add more comments on what these tests are trying to test.
Otherwise, they look like just random numbers pulled out of a hat. Some are, but
most are not. The tests are trying to get coverage of all of the branches in the
routines.

It's probably easiest to pull the comments from the original test suite in
www/~rtoy/V8/tests.js.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:197: assertEquals(two_32, Math.sin(two_32));
The two_32 and two_28 tests are to verify that sin(x) = x, exactly.  It's really
hard to guess that from just these two tests.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:200: assertEquals(-0.3826834323650898,
-Math.sin(Math.PI/8));
I think these 2 tests are intended to test case of sin(x) for x < pi/4, which is
one of the branches in MathSin

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:204: assertEquals(1, Math.cos(-two_32));
These two are for verifying cos(x) = 1 exactly for |x| < 2^-27.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:255: assertEquals(2.910566692924059e11,
Math.tan(1048575/2*Math.PI));
Now that the full Payne-Hanek reduction is implemented you should add a couple
of tests for that. (My test suite didn't include it because I didn't implement
Payne-Hanek.)

Something like

assertEquals(0.377820109360752e0, Math.sin(Math.pow(2, 120)));
assertEquals(-0.9258790228548379e0, Math.cos(Math.pow(2, 120)));
assertEquals(-0.40806638884180424e0, Math.tan(Math.pow(2 ,120)));

You probably want to test -2^120 as well.

Another interesting test is pi*2^120 and it's negative.

assertEquals(0.9506464744956261e0, Math.PI * Math.sin(Math.pow(2,120)));
assertEquals(-0.310276135932232e0, Math.PI * Math.cos(Math.pow(2, 120)))
assertEquals(-3.063872352410817e0, Math.PI * Math.tan(Math.pow(2, 120)))

Also, tests.js includes some tests of reduction for various multiples of pi/2.
It might be useful to include those as well because they test various branches
on the reduction code where differing number of bits of pi are needed.

[Raymond Toy, 2014-07-24 18:08:44]

https://codereview.chromium.org/411263004/diff/20001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode188
src/math.js:188: macro REMPIO2(X)
Does this have to be a macro? Dropping this big chunk of code all over would
seem to hurt the instruction cache on mobile devices.  Is there a huge loss in
performance if this were a regular function?

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode381
src/math.js:381: if (%_IsMinusZero(x)) return x;
Is this test for -0 necessary? KERNELSIN should be able to handle this.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode402
src/math.js:402: if (%_IsMinusZero(x)) return x;
Is this test for -0 really necessary? If KernelTan doesn't work for -0, then
KernelTan is probably broken.

[Raymond Toy, 2014-07-24 18:13:18]

A question on how sin(Infinity) or sin(Nan) is handled.

It seems that for these values, we have to go all the way through to the
Payne-Hanek routine in C to get it to return NaN, which is then passed into the
kernel routines.

While NaN and Infinity are relatively rare, it's been my experience that once
NaN is generated, it just propagates everywhere forever, so now all trig
functions will be blazingly slow.

Would it make sense to add the special case to REMPIO2 instead? And maybe inform
MathSin, MathCos, and MathTan that REMPIO2 returned NaN so the kernel routines
aren't called?

[Raymond Toy, 2014-07-24 22:41:50]

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js
File test/mjsunit/sin-cos.js (right):

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:194: // Tests for Math.sin for |x| < pi/4
The spec also lists specific values for various arguments. You should add tests
for these as well.

[Yang, 2014-07-28 11:29:00]

https://codereview.chromium.org/411263004/diff/20001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode188
src/math.js:188: macro REMPIO2(X)
On 2014/07/24 18:08:44, Raymond Toy wrote:
> Does this have to be a macro? Dropping this big chunk of code all over would
> seem to hurt the instruction cache on mobile devices.  Is there a huge loss in
> performance if this were a regular function?

Yes. Calling a function with double arguments in V8 requires boxing it into a
HeapNumber. We also have three return values instead of one. So inlining it as a
macro is indeed (I tested) faster.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode196
src/math.js:196: var z = X - kTrig[1];
On 2014/07/24 16:51:11, Raymond Toy wrote:
> Readability would be improved if the kTrig values used here were actual
> constants. Now it's pretty hard to know that kTrig[2] was pio2_1t. (That's not
> really clear either, but at least it hints that the value is probably related
to
> pi/2, which it is.)

That is true. However, given that we currently don't have constant pools for
ia32 and x64, this speeds up loading double constants a lot. I would like to
keep it this way. I'll add a comment to explain where to look it up.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode262
src/math.js:262: // Sine for [-pi/4, pi/4], pi/4 ~ 0.7854
On 2014/07/24 16:51:11, Raymond Toy wrote:
> I think it would be beneficial to add the original fdlibm comment on how this
> routine works.

Done.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode268
src/math.js:268: return (X0 - ((z * (0.5 * X1 - v * r) - X1) - v * kTrig[7]))
SIGN;
On 2014/07/24 16:51:11, Raymond Toy wrote:
> The original kernel_sin function had a slight optimization where if x1 were
> known to be zero, some of the computations could be removed. It doesn't affect
> the result to do the extra computations, but there are extra computations.
> 
> Is it not needed?

I think I checked this. And I rechecked. In neither micro-benchmarks nor popular
JS Benchmarks this buys anything.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode269
src/math.js:269: endmacro
On 2014/07/24 16:51:12, Raymond Toy wrote:
> I think readability would be enhanced if you had a separate array for the
> coefficients here instead of using kTrig[7-12].  Same goes for the cosine and
> tangent coefficients below.
> 
> Is it really necessary to use just one array?

Acknowledged.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode275
src/math.js:275: var r = z * (kTrig[13] + z * (kTrig[14] + z * (kTrig[15] +
On 2014/07/24 16:51:11, Raymond Toy wrote:
> Can we use a separate array for kTrig values here to indicate that these are
the
> coefficients of the cos polynomial approximation?

There is not much readability to be gained here imo. I'd rather clean this up
once we have constant pools for intel platforms as well instead of splitting it
up into smaller constant arrays.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode281
src/math.js:281: if (ix > 0x3fe90000) {
On 2014/07/24 16:51:12, Raymond Toy wrote:
> I think it's really useful to include the fdlibm comment about how this
function
> works. Without it, it's really impossible to understand why there's a test
here
> for 0x3fe90000 and what the code is trying to do with that.

imo the comment in your port doesn't explain much either except for pointing to
the original fdlibm. So I think adding that line doesn't add much value.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode291
src/math.js:291: // Tangent for [-pi/4, pi/4], pi/4 ~ 0.7854
On 2014/07/24 16:51:12, Raymond Toy wrote:
> Include original fdlibm comment. Without that it's really hard to figure out
> that even though this is called KernelTan, it can return either tan(x) or
cot(x)
> depending on the value of returnTan.

Done.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode331
src/math.js:331: var r = kTrig[20] + w * (kTrig[22] + w * (kTrig[24] +
On 2014/07/24 16:51:11, Raymond Toy wrote:
> Can we use a separate array for the coefficients instead of kTrig?

Acknowledged.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode355
src/math.js:355: function MathSinSlow(x) {
On 2014/07/24 16:51:12, Raymond Toy wrote:
> I think MathSinAccurate is a better name. Or maybe MathSinWithPIReduction.

This function is not more accurate. The difference is that MathSin handles the
fast case, which is inlined, while this handles the slow case. We have this
naming scheme elsewhere, so I'd like to keep it this way.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode365
src/math.js:365: function MathCosSlow(x) {
On 2014/07/24 16:51:11, Raymond Toy wrote:
> Similar comment as MathSinSlow.

Acknowledged.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode381
src/math.js:381: if (%_IsMinusZero(x)) return x;
On 2014/07/24 18:08:44, Raymond Toy wrote:
> Is this test for -0 necessary? KERNELSIN should be able to handle this.

You are right. Removed.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode402
src/math.js:402: if (%_IsMinusZero(x)) return x;
On 2014/07/24 18:08:43, Raymond Toy wrote:
> Is this test for -0 really necessary? If KernelTan doesn't work for -0, then
> KernelTan is probably broken.

Done.

https://codereview.chromium.org/411263004/diff/20001/src/rempio2.cc
File src/rempio2.cc (right):

https://codereview.chromium.org/411263004/diff/20001/src/rempio2.cc#newcode61
src/rempio2.cc:61: };
On 2014/07/24 16:51:12, Raymond Toy wrote:
> I think readability would be vastly improved if you didn't smash all of these
> constants into one giant array.  At the very least, the S1-S6, C1-C6 and
T0-T12
> values should be in their own arrays.

Like previously mentioned. This should all go away once we have constant pools
for all platforms.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js
File test/mjsunit/sin-cos.js (right):

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:197: assertEquals(two_32, Math.sin(two_32));
On 2014/07/24 16:51:12, Raymond Toy wrote:
> The two_32 and two_28 tests are to verify that sin(x) = x, exactly.  It's
really
> hard to guess that from just these two tests.

Done.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:200: assertEquals(-0.3826834323650898,
-Math.sin(Math.PI/8));
On 2014/07/24 16:51:12, Raymond Toy wrote:
> I think these 2 tests are intended to test case of sin(x) for x < pi/4, which
is
> one of the branches in MathSin

Done.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:204: assertEquals(1, Math.cos(-two_32));
On 2014/07/24 16:51:12, Raymond Toy wrote:
> These two are for verifying cos(x) = 1 exactly for |x| < 2^-27.

Done.

https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:255: assertEquals(2.910566692924059e11,
Math.tan(1048575/2*Math.PI));
On 2014/07/24 16:51:12, Raymond Toy wrote:
> Now that the full Payne-Hanek reduction is implemented you should add a couple
> of tests for that. (My test suite didn't include it because I didn't implement
> Payne-Hanek.)
> 
> Something like
> 
> assertEquals(0.377820109360752e0, Math.sin(Math.pow(2, 120)));
> assertEquals(-0.9258790228548379e0, Math.cos(Math.pow(2, 120)));
> assertEquals(-0.40806638884180424e0, Math.tan(Math.pow(2 ,120)));
> 
> You probably want to test -2^120 as well.
> 
> Another interesting test is pi*2^120 and it's negative.
> 
> assertEquals(0.9506464744956261e0, Math.PI * Math.sin(Math.pow(2,120)));
> assertEquals(-0.310276135932232e0, Math.PI * Math.cos(Math.pow(2, 120)))
> assertEquals(-3.063872352410817e0, Math.PI * Math.tan(Math.pow(2, 120)))
> 
> Also, tests.js includes some tests of reduction for various multiples of pi/2.
> It might be useful to include those as well because they test various branches
> on the reduction code where differing number of bits of pi are needed.
> 

Done.

[Yang, 2014-07-28 12:22:19]

On 2014/07/28 11:29:00, Yang wrote:
> https://codereview.chromium.org/411263004/diff/20001/src/math.js
> File src/math.js (right):
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode188
> src/math.js:188: macro REMPIO2(X)
> On 2014/07/24 18:08:44, Raymond Toy wrote:
> > Does this have to be a macro? Dropping this big chunk of code all over would
> > seem to hurt the instruction cache on mobile devices.  Is there a huge loss
in
> > performance if this were a regular function?
> 
> Yes. Calling a function with double arguments in V8 requires boxing it into a
> HeapNumber. We also have three return values instead of one. So inlining it as
a
> macro is indeed (I tested) faster.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode196
> src/math.js:196: var z = X - kTrig[1];
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > Readability would be improved if the kTrig values used here were actual
> > constants. Now it's pretty hard to know that kTrig[2] was pio2_1t. (That's
not
> > really clear either, but at least it hints that the value is probably
related
> to
> > pi/2, which it is.)
> 
> That is true. However, given that we currently don't have constant pools for
> ia32 and x64, this speeds up loading double constants a lot. I would like to
> keep it this way. I'll add a comment to explain where to look it up.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode262
> src/math.js:262: // Sine for [-pi/4, pi/4], pi/4 ~ 0.7854
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > I think it would be beneficial to add the original fdlibm comment on how
this
> > routine works.
> 
> Done.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode268
> src/math.js:268: return (X0 - ((z * (0.5 * X1 - v * r) - X1) - v * kTrig[7]))
> SIGN;
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > The original kernel_sin function had a slight optimization where if x1 were
> > known to be zero, some of the computations could be removed. It doesn't
affect
> > the result to do the extra computations, but there are extra computations.
> > 
> > Is it not needed?
> 
> I think I checked this. And I rechecked. In neither micro-benchmarks nor
popular
> JS Benchmarks this buys anything.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode269
> src/math.js:269: endmacro
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think readability would be enhanced if you had a separate array for the
> > coefficients here instead of using kTrig[7-12].  Same goes for the cosine
and
> > tangent coefficients below.
> > 
> > Is it really necessary to use just one array?
> 
> Acknowledged.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode275
> src/math.js:275: var r = z * (kTrig[13] + z * (kTrig[14] + z * (kTrig[15] +
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > Can we use a separate array for kTrig values here to indicate that these are
> the
> > coefficients of the cos polynomial approximation?
> 
> There is not much readability to be gained here imo. I'd rather clean this up
> once we have constant pools for intel platforms as well instead of splitting
it
> up into smaller constant arrays.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode281
> src/math.js:281: if (ix > 0x3fe90000) {
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think it's really useful to include the fdlibm comment about how this
> function
> > works. Without it, it's really impossible to understand why there's a test
> here
> > for 0x3fe90000 and what the code is trying to do with that.
> 
> imo the comment in your port doesn't explain much either except for pointing
to
> the original fdlibm. So I think adding that line doesn't add much value.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode291
> src/math.js:291: // Tangent for [-pi/4, pi/4], pi/4 ~ 0.7854
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > Include original fdlibm comment. Without that it's really hard to figure out
> > that even though this is called KernelTan, it can return either tan(x) or
> cot(x)
> > depending on the value of returnTan.
> 
> Done.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode331
> src/math.js:331: var r = kTrig[20] + w * (kTrig[22] + w * (kTrig[24] +
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > Can we use a separate array for the coefficients instead of kTrig?
> 
> Acknowledged.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode355
> src/math.js:355: function MathSinSlow(x) {
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think MathSinAccurate is a better name. Or maybe MathSinWithPIReduction.
> 
> This function is not more accurate. The difference is that MathSin handles the
> fast case, which is inlined, while this handles the slow case. We have this
> naming scheme elsewhere, so I'd like to keep it this way.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode365
> src/math.js:365: function MathCosSlow(x) {
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > Similar comment as MathSinSlow.
> 
> Acknowledged.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode381
> src/math.js:381: if (%_IsMinusZero(x)) return x;
> On 2014/07/24 18:08:44, Raymond Toy wrote:
> > Is this test for -0 necessary? KERNELSIN should be able to handle this.
> 
> You are right. Removed.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode402
> src/math.js:402: if (%_IsMinusZero(x)) return x;
> On 2014/07/24 18:08:43, Raymond Toy wrote:
> > Is this test for -0 really necessary? If KernelTan doesn't work for -0, then
> > KernelTan is probably broken.
> 
> Done.
> 
> https://codereview.chromium.org/411263004/diff/20001/src/rempio2.cc
> File src/rempio2.cc (right):
> 
> https://codereview.chromium.org/411263004/diff/20001/src/rempio2.cc#newcode61
> src/rempio2.cc:61: };
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think readability would be vastly improved if you didn't smash all of
these
> > constants into one giant array.  At the very least, the S1-S6, C1-C6 and
> T0-T12
> > values should be in their own arrays.
> 
> Like previously mentioned. This should all go away once we have constant pools
> for all platforms.
> 
> https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js
> File test/mjsunit/sin-cos.js (right):
> 
>
https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
> test/mjsunit/sin-cos.js:197: assertEquals(two_32, Math.sin(two_32));
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > The two_32 and two_28 tests are to verify that sin(x) = x, exactly.  It's
> really
> > hard to guess that from just these two tests.
> 
> Done.
> 
>
https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
> test/mjsunit/sin-cos.js:200: assertEquals(-0.3826834323650898,
> -Math.sin(Math.PI/8));
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think these 2 tests are intended to test case of sin(x) for x < pi/4,
which
> is
> > one of the branches in MathSin
> 
> Done.
> 
>
https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
> test/mjsunit/sin-cos.js:204: assertEquals(1, Math.cos(-two_32));
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > These two are for verifying cos(x) = 1 exactly for |x| < 2^-27.
> 
> Done.
> 
>
https://codereview.chromium.org/411263004/diff/20001/test/mjsunit/sin-cos.js#...
> test/mjsunit/sin-cos.js:255: assertEquals(2.910566692924059e11,
> Math.tan(1048575/2*Math.PI));
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > Now that the full Payne-Hanek reduction is implemented you should add a
couple
> > of tests for that. (My test suite didn't include it because I didn't
implement
> > Payne-Hanek.)
> > 
> > Something like
> > 
> > assertEquals(0.377820109360752e0, Math.sin(Math.pow(2, 120)));
> > assertEquals(-0.9258790228548379e0, Math.cos(Math.pow(2, 120)));
> > assertEquals(-0.40806638884180424e0, Math.tan(Math.pow(2 ,120)));
> > 
> > You probably want to test -2^120 as well.
> > 
> > Another interesting test is pi*2^120 and it's negative.
> > 
> > assertEquals(0.9506464744956261e0, Math.PI * Math.sin(Math.pow(2,120)));
> > assertEquals(-0.310276135932232e0, Math.PI * Math.cos(Math.pow(2, 120)))
> > assertEquals(-3.063872352410817e0, Math.PI * Math.tan(Math.pow(2, 120)))
> > 
> > Also, tests.js includes some tests of reduction for various multiples of
pi/2.
> > It might be useful to include those as well because they test various
branches
> > on the reduction code where differing number of bits of pi are needed.
> > 
> 
> Done.

Moved the parts ported from fdlibm to third_party/fdlibm.

[Raymond Toy, 2014-07-30 19:55:05]

https://codereview.chromium.org/411263004/diff/20001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode188
src/math.js:188: macro REMPIO2(X)
On 2014/07/28 11:28:59, Yang wrote:
> On 2014/07/24 18:08:44, Raymond Toy wrote:
> > Does this have to be a macro? Dropping this big chunk of code all over would
> > seem to hurt the instruction cache on mobile devices.  Is there a huge loss
in
> > performance if this were a regular function?
> 
> Yes. Calling a function with double arguments in V8 requires boxing it into a
> HeapNumber. We also have three return values instead of one. So inlining it as
a
> macro is indeed (I tested) faster.

Ok, that's not too surprising if your tests stress trig functions.

I was curious on your thoughts about blowing out the instruction cache on low
end devices, but I don't have any good tests for this.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode196
src/math.js:196: var z = X - kTrig[1];
On 2014/07/28 11:28:59, Yang wrote:
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > Readability would be improved if the kTrig values used here were actual
> > constants. Now it's pretty hard to know that kTrig[2] was pio2_1t. (That's
not
> > really clear either, but at least it hints that the value is probably
related
> to
> > pi/2, which it is.)
> 
> That is true. However, given that we currently don't have constant pools for
> ia32 and x64, this speeds up loading double constants a lot. I would like to
> keep it this way. I'll add a comment to explain where to look it up.

Could you give symbolic names for the indices? So instead of kTrig[2], use
kTrig[pio2_1t]? Doesn't solve all readability issues, but does make it a bit
more obvious.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode231
src/math.js:231: if (ix - (%_DoubleHi(y0) & 0x7ff00000) > 0x1000000) {
It would certainly be good to document what this is doing. I know the original
didn't, but that doesn't mean we shouldn't. I can help with this.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode238
src/math.js:238: if (ix - (%_DoubleHi(y0) & 0x7ff00000) > 0x3100000) {
Same comment as for line 231.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode268
src/math.js:268: return (X0 - ((z * (0.5 * X1 - v * r) - X1) - v * kTrig[7]))
SIGN;
On 2014/07/28 11:28:59, Yang wrote:
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > The original kernel_sin function had a slight optimization where if x1 were
> > known to be zero, some of the computations could be removed. It doesn't
affect
> > the result to do the extra computations, but there are extra computations.
> > 
> > Is it not needed?
> 
> I think I checked this. And I rechecked. In neither micro-benchmarks nor
popular
> JS Benchmarks this buys anything.

Fair enough.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode275
src/math.js:275: var r = z * (kTrig[13] + z * (kTrig[14] + z * (kTrig[15] +
On 2014/07/28 11:28:59, Yang wrote:
> On 2014/07/24 16:51:11, Raymond Toy wrote:
> > Can we use a separate array for kTrig values here to indicate that these are
> the
> > coefficients of the cos polynomial approximation?
> 
> There is not much readability to be gained here imo. I'd rather clean this up
> once we have constant pools for intel platforms as well instead of splitting
it
> up into smaller constant arrays.

I have no problems with the various random constants placed in kTrig. I think,
however, that readability is vastly improved if the coefficients of the
approximations are put in their own arrays. 

When doing the first pass over this, I had to go hunt around to figure out what
kTrig[13] was. If you had just had C[0] (or is it C[1])? then it would have been
pretty obvious what is happening here.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode277
src/math.js:277: if (ix < 0x3fd33333) {
Add comment that ix < 0x3fd33333 is testing for |x| ~< 0.3, so it's easier to
match the function documentation.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode281
src/math.js:281: if (ix > 0x3fe90000) {
On 2014/07/28 11:28:59, Yang wrote:
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think it's really useful to include the fdlibm comment about how this
> function
> > works. Without it, it's really impossible to understand why there's a test
> here
> > for 0x3fe90000 and what the code is trying to do with that.
> 
> imo the comment in your port doesn't explain much either except for pointing
to
> the original fdlibm. So I think adding that line doesn't add much value.

No, but at least it makes it clear that ix > 0x3fe90000 is testing the case for
x > 0.78125. And that is certainly easier to understand than the former and
makes it easier to match up with the function documentation.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode300
src/math.js:300: if (((ix | %_DoubleLo(x)) | (returnTan + 1)) == 0) {
This might be more obvious if we said abs(x) == 0 and returnTan = -1.  I think
that's what this is trying to test so we can return Infinity for cot(0).

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode331
src/math.js:331: var r = kTrig[20] + w * (kTrig[22] + w * (kTrig[24] +
Add back the comment. This would be unexpected from the function description. 
The comment was:

    //
    // Break x^5*(T[1]+x^2*T[2]+...) into
    // x^5(T[1]+x^4*T[3]+...+x^20*T[11]) +
    // x^5(x^2*(T[2]+x^4*T[4]+...+x^22*[T12]))

I do not know why fdlibm does this.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode339
src/math.js:339: if (ix >= 0x3fe59428) {
Document what 0x3fe59428 is.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode340
src/math.js:340: return (1 - ((hx >> 30) & 2)) *
Describe what ((hx >> 30) & 2) is doing.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode355
src/math.js:355: function MathSinSlow(x) {
On 2014/07/28 11:28:59, Yang wrote:
> On 2014/07/24 16:51:12, Raymond Toy wrote:
> > I think MathSinAccurate is a better name. Or maybe MathSinWithPIReduction.
> 
> This function is not more accurate. The difference is that MathSin handles the
> fast case, which is inlined, while this handles the slow case. We have this
> naming scheme elsewhere, so I'd like to keep it this way.

Sure, that makes sense.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js
File test/mjsunit/sin-cos.js (right):

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:208: // cos(pi/20) =
sqrt(sqrt(2)*sqrt(sqrt(5)+5)+4)/2^(3/2)
This is tests the |x| < 0.3 branch in KERNELCOS.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:211: assertEquals(0.7100338835660797,
Math.cos(0.78125));
Lines 210-211 are testing the KERNELCOS test for x > 0.78125 and x <= 0.78125

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:212: // cos(pi/8) = sqrt(sqrt(2)+1)/2^(3/4)
This tests the |x| > 0.3 branch in KERNELCOS

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:217: assertEquals("Infinity", String(1/Math.tan(0.0)));
For the sin test, you used Infinity and +0.0. I think the tan test and the sin
test should be consistent here.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:222: assertEquals(0.8211418015898941, Math.tan(11/16));
Lines 222-223 tests the case |x| > 0.67434 in KERNELTAN.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:224: assertEquals(0.41421356237309503, Math.tan(Math.PI
/ 8));
This tests the case in KERNELTAN for |x| < 0.67434.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:231: assertEquals(1.2246467991473532e-16,
Math.sin(Math.PI));
Verifies Math.sin(Math.PI) is not zero because Math.PI is not pi so Math.sin
can't be zero.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:236: assertEquals(-0.7071067811865479, Math.sin(13/4 *
Math.PI));
I think lines 233-236 are intended to test the 4 various cases in sin calling
kernelsin and kernelcos and some of the branches in the pi reduction code.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:243: assertEquals(0.7073882691671998, Math.cos(0.785));
Lines 241-243 are more tests on the three possible branches in KERNELCOS.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:244: assertEquals(6.123233995736766e-17,
Math.cos(Math.PI/2));
Verifies Math.cos(Math.PI/2) is not 0 because Math.PI/2 is not pi/2 so Math.cos
can't be zero.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:248: assertEquals(-0.7071067811865471, Math.cos(13/4 *
Math.PI));
Lines 245-248 tests the four possible cases in computing cos using KERNELCOS and
KERNELSIN.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:254: assertEquals(1.633123935319537e16,
Math.tan(Math.PI/2));
Verifies Math.tan(Math.PI/2) is not an infinity because Math.PI/2 is not pi/2 so
Math.tan can't be an infinity.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:256: assertEquals(0.8211418015898941, Math.tan(11/16));
255-256 tests (again) the various branches in KERNELTAN. Maybe line 256 should
be removed since we cover that in line 222.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:259: assertEquals(0.9999999999999994,
Math.tan(9/4*Math.PI));
Lines 258-259 verifies the two cases of tan calling KERNELTAN to compute the tan
or cot.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:269: assertEquals(0.40806638884180424e0,
Math.tan(-Math.pow(2, 120)));
I still think some test from special cases listed in the spec should be added
here.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/READM...
File third_party/fdlibm/README.v8 (right):

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/READM...
third_party/fdlibm/README.v8:11: This is used to provide a precise
implementation for trigonometric functions
I think "accurate" is the correct word here, not precise.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
File third_party/fdlibm/fdlibm.js (right):

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:115: //    |  x                                    
          |
Please fix the alignment. The exponents and right | are all messed up for me.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:147: //  |ieee_cos(x)-(1-.5*x +C1*x +C2*x +C3*x
+C4*x +C5*x  +C6*x  )| <= 2
Fix alignment of exponents.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:200: //     |----- - (1+T1*x +T2*x +.... +T13*x   
)| <= 2
Line up exponents correctly.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:283: REMPIO2(x);
Since we're doing the slow path anyway, I think it's useful to check for
infinity and NaN here and do a quick return instead of trying to compute with
infinity and NaN all the way through.

Same comment for MathCosSlow.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:330: REMPIO2(x);
Like for MathSinSlow and MathCosSlow, I think you should have a quick exit here
for Infinity and NaN instead of propagating them (slowly) all the way through
the reduction and tan computation.

[Yang, 2014-08-01 07:29:57]

https://codereview.chromium.org/411263004/diff/20001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode196
src/math.js:196: var z = X - kTrig[1];
On 2014/07/30 19:55:04, Raymond Toy wrote:
> On 2014/07/28 11:28:59, Yang wrote:
> > On 2014/07/24 16:51:11, Raymond Toy wrote:
> > > Readability would be improved if the kTrig values used here were actual
> > > constants. Now it's pretty hard to know that kTrig[2] was pio2_1t. (That's
> not
> > > really clear either, but at least it hints that the value is probably
> related
> > to
> > > pi/2, which it is.)
> > 
> > That is true. However, given that we currently don't have constant pools for
> > ia32 and x64, this speeds up loading double constants a lot. I would like to
> > keep it this way. I'll add a comment to explain where to look it up.
> 
> Could you give symbolic names for the indices? So instead of kTrig[2], use
> kTrig[pio2_1t]? Doesn't solve all readability issues, but does make it a bit
> more obvious.

Done with macros

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode231
src/math.js:231: if (ix - (%_DoubleHi(y0) & 0x7ff00000) > 0x1000000) {
On 2014/07/30 19:55:04, Raymond Toy wrote:
> It would certainly be good to document what this is doing. I know the original
> didn't, but that doesn't mean we shouldn't. I can help with this.

I don't think we should be spending time documenting third_party code, let alone
one with one-letter variable names.

Feel free to contribute, but I'd like to not to have this CL blocked by this.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode238
src/math.js:238: if (ix - (%_DoubleHi(y0) & 0x7ff00000) > 0x3100000) {
On 2014/07/30 19:55:03, Raymond Toy wrote:
> Same comment as for line 231.

Acknowledged.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode275
src/math.js:275: var r = z * (kTrig[13] + z * (kTrig[14] + z * (kTrig[15] +
On 2014/07/30 19:55:04, Raymond Toy wrote:
> On 2014/07/28 11:28:59, Yang wrote:
> > On 2014/07/24 16:51:11, Raymond Toy wrote:
> > > Can we use a separate array for kTrig values here to indicate that these
are
> > the
> > > coefficients of the cos polynomial approximation?
> > 
> > There is not much readability to be gained here imo. I'd rather clean this
up
> > once we have constant pools for intel platforms as well instead of splitting
> it
> > up into smaller constant arrays.
> 
> I have no problems with the various random constants placed in kTrig. I think,
> however, that readability is vastly improved if the coefficients of the
> approximations are put in their own arrays. 
> 
> When doing the first pass over this, I had to go hunt around to figure out
what
> kTrig[13] was. If you had just had C[0] (or is it C[1])? then it would have
been
> pretty obvious what is happening here.

Done with macros.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode277
src/math.js:277: if (ix < 0x3fd33333) {
On 2014/07/30 19:55:03, Raymond Toy wrote:
> Add comment that ix < 0x3fd33333 is testing for |x| ~< 0.3, so it's easier to
> match the function documentation.

Done.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode281
src/math.js:281: if (ix > 0x3fe90000) {
On 2014/07/30 19:55:03, Raymond Toy wrote:
> On 2014/07/28 11:28:59, Yang wrote:
> > On 2014/07/24 16:51:12, Raymond Toy wrote:
> > > I think it's really useful to include the fdlibm comment about how this
> > function
> > > works. Without it, it's really impossible to understand why there's a test
> > here
> > > for 0x3fe90000 and what the code is trying to do with that.
> > 
> > imo the comment in your port doesn't explain much either except for pointing
> to
> > the original fdlibm. So I think adding that line doesn't add much value.
> 
> No, but at least it makes it clear that ix > 0x3fe90000 is testing the case
for
> x > 0.78125. And that is certainly easier to understand than the former and
> makes it easier to match up with the function documentation.

Done.

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode300
src/math.js:300: if (((ix | %_DoubleLo(x)) | (returnTan + 1)) == 0) {
On 2014/07/30 19:55:04, Raymond Toy wrote:
> This might be more obvious if we said abs(x) == 0 and returnTan = -1.  I think
> that's what this is trying to test so we can return Infinity for cot(0).

I don't think I changed anything here from your port. I added a comment to
clarify.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js
File test/mjsunit/sin-cos.js (right):

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:208: // cos(pi/20) =
sqrt(sqrt(2)*sqrt(sqrt(5)+5)+4)/2^(3/2)
On 2014/07/30 19:55:04, Raymond Toy wrote:
> This is tests the |x| < 0.3 branch in KERNELCOS.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:211: assertEquals(0.7100338835660797,
Math.cos(0.78125));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> Lines 210-211 are testing the KERNELCOS test for x > 0.78125 and x <= 0.78125

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:212: // cos(pi/8) = sqrt(sqrt(2)+1)/2^(3/4)
On 2014/07/30 19:55:05, Raymond Toy wrote:
> This tests the |x| > 0.3 branch in KERNELCOS

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:217: assertEquals("Infinity", String(1/Math.tan(0.0)));
On 2014/07/30 19:55:05, Raymond Toy wrote:
> For the sin test, you used Infinity and +0.0. I think the tan test and the sin
> test should be consistent here.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:222: assertEquals(0.8211418015898941, Math.tan(11/16));
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Lines 222-223 tests the case |x| > 0.67434 in KERNELTAN.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:224: assertEquals(0.41421356237309503, Math.tan(Math.PI
/ 8));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> This tests the case in KERNELTAN for |x| < 0.67434.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:231: assertEquals(1.2246467991473532e-16,
Math.sin(Math.PI));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> Verifies Math.sin(Math.PI) is not zero because Math.PI is not pi so Math.sin
> can't be zero.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:236: assertEquals(-0.7071067811865479, Math.sin(13/4 *
Math.PI));
On 2014/07/30 19:55:05, Raymond Toy wrote:
> I think lines 233-236 are intended to test the 4 various cases in sin calling
> kernelsin and kernelcos and some of the branches in the pi reduction code.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:243: assertEquals(0.7073882691671998, Math.cos(0.785));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> Lines 241-243 are more tests on the three possible branches in KERNELCOS.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:244: assertEquals(6.123233995736766e-17,
Math.cos(Math.PI/2));
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Verifies Math.cos(Math.PI/2) is not 0 because Math.PI/2 is not pi/2 so
Math.cos
> can't be zero.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:248: assertEquals(-0.7071067811865471, Math.cos(13/4 *
Math.PI));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> Lines 245-248 tests the four possible cases in computing cos using KERNELCOS
and
> KERNELSIN.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:254: assertEquals(1.633123935319537e16,
Math.tan(Math.PI/2));
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Verifies Math.tan(Math.PI/2) is not an infinity because Math.PI/2 is not pi/2
so
> Math.tan can't be an infinity.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:256: assertEquals(0.8211418015898941, Math.tan(11/16));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> 255-256 tests (again) the various branches in KERNELTAN. Maybe line 256 should
> be removed since we cover that in line 222.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:259: assertEquals(0.9999999999999994,
Math.tan(9/4*Math.PI));
On 2014/07/30 19:55:04, Raymond Toy wrote:
> Lines 258-259 verifies the two cases of tan calling KERNELTAN to compute the
tan
> or cot.

Done.

https://codereview.chromium.org/411263004/diff/80001/test/mjsunit/sin-cos.js#...
test/mjsunit/sin-cos.js:269: assertEquals(0.40806638884180424e0,
Math.tan(-Math.pow(2, 120)));
On 2014/07/30 19:55:05, Raymond Toy wrote:
> I still think some test from special cases listed in the spec should be added
> here.

Not sure what spec you are referring to here.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/READM...
File third_party/fdlibm/README.v8 (right):

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/READM...
third_party/fdlibm/README.v8:11: This is used to provide a precise
implementation for trigonometric functions
On 2014/07/30 19:55:05, Raymond Toy wrote:
> I think "accurate" is the correct word here, not precise.

Done.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
File third_party/fdlibm/fdlibm.js (right):

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:115: //    |  x                                    
          |
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Please fix the alignment. The exponents and right | are all messed up for me.

I aligned it to visually match the port you gave me (it contained tabs).

I re-aligned it to match the original.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:147: //  |ieee_cos(x)-(1-.5*x +C1*x +C2*x +C3*x
+C4*x +C5*x  +C6*x  )| <= 2
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Fix alignment of exponents.

Done.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:200: //     |----- - (1+T1*x +T2*x +.... +T13*x   
)| <= 2
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Line up exponents correctly.

Done.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:283: REMPIO2(x);
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Since we're doing the slow path anyway, I think it's useful to check for
> infinity and NaN here and do a quick return instead of trying to compute with
> infinity and NaN all the way through.
> 
> Same comment for MathCosSlow.

We just generally don't care about performance for NaN and Infinities. Adding a
special case for it at the (even though small) expense of normal computation
makes no sense.

https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlib...
third_party/fdlibm/fdlibm.js:330: REMPIO2(x);
On 2014/07/30 19:55:05, Raymond Toy wrote:
> Like for MathSinSlow and MathCosSlow, I think you should have a quick exit
here
> for Infinity and NaN instead of propagating them (slowly) all the way through
> the reduction and tan computation.

Done.

[Raymond Toy, 2014-08-01 14:06:01]

https://codereview.chromium.org/411263004/diff/20001/src/math.js
File src/math.js (right):

https://codereview.chromium.org/411263004/diff/20001/src/math.js#newcode231
src/math.js:231: if (ix - (%_DoubleHi(y0) & 0x7ff00000) > 0x1000000) {
On 2014/08/01 07:29:55, Yang wrote:
> On 2014/07/30 19:55:04, Raymond Toy wrote:
> > It would certainly be good to document what this is doing. I know the
original
> > didn't, but that doesn't mean we shouldn't. I can help with this.
> 
> I don't think we should be spending time documenting third_party code, let
alone
> one with one-letter variable names.

I would agree if this were in fact the original code, but it's now a translation
of the C code to Javascript.

> 
> Feel free to contribute, but I'd like to not to have this CL blocked by this.

Fair enough.

FWIW, I think the %_DoubleHi & 0x7ff00000 is extracting the (biased) exponent
from the y0, and subtracting that from ix gives a rough idea of how close ix is
to a multiple of pi.  But I'm not sure.

https://codereview.chromium.org/411263004/diff/100001/test/mjsunit/sin-cos.js
File test/mjsunit/sin-cos.js (right):

https://codereview.chromium.org/411263004/diff/100001/test/mjsunit/sin-cos.js...
test/mjsunit/sin-cos.js:223: // Test KERNELTAN for |x| > 0.67434.
Comment on line 223 should probably be moved to before line 227.

[Raymond Toy, 2014-08-01 14:08:00]

lgtm

But I didn't review the most recent changes to js2c.js; you might need someone
else to review that.

Thanks for getting this all integrated into V8!

[jochen (gone - plz use gerrit), 2014-08-04 07:09:41]

https://codereview.chromium.org/411263004/diff/100001/third_party/fdlibm/READ...
File third_party/fdlibm/README.v8 (right):

https://codereview.chromium.org/411263004/diff/100001/third_party/fdlibm/READ...
third_party/fdlibm/README.v8:6: License File: LICENSE.
this file is missing?

[Yang, 2014-08-04 07:11:30]

On 2014/08/04 07:09:41, jochen wrote:
>
https://codereview.chromium.org/411263004/diff/100001/third_party/fdlibm/READ...
> File third_party/fdlibm/README.v8 (right):
> 
>
https://codereview.chromium.org/411263004/diff/100001/third_party/fdlibm/READ...
> third_party/fdlibm/README.v8:6: License File: LICENSE.
> this file is missing?

You are completely right. Forgot to git add. Updated.

[jochen (gone - plz use gerrit), 2014-08-04 07:52:12]

license and readme lgtm

[Yang, 2014-08-06 10:33:36]

Committed patchset #9 manually as 22918 (presubmit successful).