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#newcode197
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#newcode200
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#newcode204
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#newcode255
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.
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.
