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#newcode208
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#newcode211
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#newcode212
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#newcode217
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#newcode222
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#newcode224
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#newcode231
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#newcode236
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#newcode243
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#newcode244
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#newcode248
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#newcode254
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#newcode256
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#newcode259
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#newcode269
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/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
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/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
|
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/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
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/fdlibm.js#newcode200
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/fdlibm.js#newcode283
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/fdlibm.js#newcode330
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.
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.
