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#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.
Moved the parts ported from fdlibm to third_party/fdlibm.
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.
