Custom call IC for Math.floor.

Custom call IC for Math.floor.

Committed: http://code.google.com/p/v8/source/detail?r=5499

[Erik Corry, 2010-09-20 11:56:50]

LGTM

http://codereview.chromium.org/3327022/diff/3001/4004
File src/ia32/disasm-ia32.cc (right):

http://codereview.chromium.org/3327022/diff/3001/4004#newcode1163
src/ia32/disasm-ia32.cc:1163: int8_t imm8 = *reinterpret_cast<int8_t*>(data +
1);
Why not just:
int imm8 = data[1];
?

http://codereview.chromium.org/3327022/diff/3001/4007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3327022/diff/3001/4007#newcode1871
src/ia32/stub-cache-ia32.cc:1871: // discards NaN.
It might be nice to detect +- zero and pass it through unchanged.  Zero is
probably reasonably common.  Once you have done that then negative numbers are
perhaps not so hard?  It seems like it's the same except you want to subtract
then add instead of adding then subtracting below and then you need to reverse
the rest too.  You can save this for another change or not do it at all :-)

http://codereview.chromium.org/3327022/diff/3001/4010
File test/mjsunit/math-floor.js (right):

http://codereview.chromium.org/3327022/diff/3001/4010#newcode31
test/mjsunit/math-floor.js:31: assertEquals(0, Math.floor(0));
This tests the Smi 0 but not the heap number 0.  Normally + will not check for a
Smi result so this should work:

x = 0.5
y = 0.5
assertEquals(x - y, Math.floor(x - y)))

http://codereview.chromium.org/3327022/diff/3001/4010#newcode32
test/mjsunit/math-floor.js:32: assertEquals(-0, Math.floor(-0));
0 == -0 so this test doesn't really work.  You need to do

1/-0, 1/Math.floor(-0)

http://codereview.chromium.org/3327022/diff/3001/4010#newcode60
test/mjsunit/math-floor.js:60: var two_30 = 1 << 30;
It would be nice to pass this in as an argument so you could easily do two_53 as
well.

[Vitaly Repeshko, 2010-09-21 12:54:49]

Thanks! The patch is now submitted.


-- Vitaly

http://codereview.chromium.org/3327022/diff/3001/4004
File src/ia32/disasm-ia32.cc (right):

http://codereview.chromium.org/3327022/diff/3001/4004#newcode1163
src/ia32/disasm-ia32.cc:1163: int8_t imm8 = *reinterpret_cast<int8_t*>(data +
1);
On 2010/09/20 11:56:50, Erik Corry wrote:
> Why not just:
> int imm8 = data[1];
> ?

Almost done. I left static_cast there to avoid unsigned to signed warnings.

http://codereview.chromium.org/3327022/diff/3001/4007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3327022/diff/3001/4007#newcode1871
src/ia32/stub-cache-ia32.cc:1871: // discards NaN.
On 2010/09/20 11:56:50, Erik Corry wrote:
> It might be nice to detect +- zero and pass it through unchanged.  Zero is
> probably reasonably common.  Once you have done that then negative numbers are
> perhaps not so hard?  It seems like it's the same except you want to subtract
> then add instead of adding then subtracting below and then you need to reverse
> the rest too.  You can save this for another change or not do it at all :-)

I filed a bug to consider extending it.

http://codereview.chromium.org/3327022/diff/3001/4010
File test/mjsunit/math-floor.js (right):

http://codereview.chromium.org/3327022/diff/3001/4010#newcode31
test/mjsunit/math-floor.js:31: assertEquals(0, Math.floor(0));
On 2010/09/20 11:56:50, Erik Corry wrote:
> This tests the Smi 0 but not the heap number 0.  Normally + will not check for
a
> Smi result so this should work:
> 
> x = 0.5
> y = 0.5
> assertEquals(x - y, Math.floor(x - y)))

Done. I left the smi test case for completeness.

http://codereview.chromium.org/3327022/diff/3001/4010#newcode32
test/mjsunit/math-floor.js:32: assertEquals(-0, Math.floor(-0));
On 2010/09/20 11:56:50, Erik Corry wrote:
> 0 == -0 so this test doesn't really work.  You need to do
> 
> 1/-0, 1/Math.floor(-0)
> 

Good catch! Fixed.

http://codereview.chromium.org/3327022/diff/3001/4010#newcode60
test/mjsunit/math-floor.js:60: var two_30 = 1 << 30;
On 2010/09/20 11:56:50, Erik Corry wrote:
> It would be nice to pass this in as an argument so you could easily do two_53
as
> well.

There are actually some differences. But even if there were none, I still think
having verbose test code would be better: it's much easier to spot an error.