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#newcode194
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#newcode197
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#newcode200
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#newcode204
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#newcode255
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.
https://codereview.chromium.org/411263004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
