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#newcode208
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#newcode211
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#newcode212
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#newcode217
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#newcode222
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#newcode224
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#newcode231
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#newcode236
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#newcode243
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#newcode244
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#newcode248
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#newcode254
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#newcode256
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#newcode259
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#newcode269
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/README.v8
File third_party/fdlibm/README.v8 (right):
https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/README.v8#newcode11
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/fdlibm.js
File third_party/fdlibm/fdlibm.js (right):
https://codereview.chromium.org/411263004/diff/80001/third_party/fdlibm/fdlibm.js#newcode115
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/fdlibm.js#newcode147
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/fdlibm.js#newcode200
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/fdlibm.js#newcode283
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/fdlibm.js#newcode330
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.
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.
