Custom call IC for Math.abs.

Custom call IC for Math.abs.

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

[antonm, 2010-09-24 15:55:59]

LGTM, but do we have enough test coverage?  test/mjsunit/abs.js is somewhat
small, for example it apparently doesn't test the case of the smallest Smi.

http://codereview.chromium.org/3446024/diff/1/2
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/1/2#newcode1578
src/ia32/macro-assembler-ia32.cc:1578: psllq(dst, 2 * kBitsPerInt - 1);
I wonder if 2 * kBitsPerInt - 1 is a proper way to encode 63 here :)  but up to
you

http://codereview.chromium.org/3446024/diff/1/4
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/1/4#newcode1967
src/ia32/stub-cache-ia32.cc:1967: if (!object->IsJSObject() || argc != 1) return
Heap::undefined_value();
pretty minor thing: maybe you shouldn't bail out if argc != 1 and only process
first argument?  That's apparently what the spec requires

[antonm, 2010-09-24 16:09:41]

Still LGTM when ported or stubbed for other platforms.

http://codereview.chromium.org/3446024/diff/6001/7005
File test/mjsunit/math-abs.js (right):

http://codereview.chromium.org/3446024/diff/6001/7005#newcode28
test/mjsunit/math-abs.js:28: // Flags: --max-new-space-size=262144
why this needed?

Do you want to merge tests from test/mjsunit/abs.js into this and nuke the
former?  Or other way around.

[Vitaly Repeshko, 2010-09-27 11:52:54]

[+Rico]

I dropped the SSE stuff, because it was not really needed. The patch is now
ported to ARM and x64. I merged mjsunit/abs.js test with the new one.


Thanks,
Vitaly

[Rico, 2010-09-27 13:18:23]

LGTM

http://codereview.chromium.org/3446024/diff/11001/12002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/11001/12002#newcode2016
src/ia32/stub-cache-ia32.cc:2016: __ mov(ebx, FieldOperand(eax,
HeapNumber::kExponentOffset));
You could potentially use an xmm register here and do (assume loaded into xmm0):
pxor(xmm1, xmm1);
subsd(xmm1, xmm0)
and(xmm0, xmm1)

http://codereview.chromium.org/3446024/diff/11001/12006
File test/mjsunit/math-abs.js (right):

http://codereview.chromium.org/3446024/diff/11001/12006#newcode28
test/mjsunit/math-abs.js:28: // Flags: --max-new-space-size=262144
Why this flag?

http://codereview.chromium.org/3446024/diff/11001/12006#newcode78
test/mjsunit/math-abs.js:78: assertEquals(two_30 - 1, Math.abs(-two_30 + 1));
Maybe add a few test cases where you call Math.abs on objects, arrays and
strings that are not actual numbers (to test that we do indeed bail out if this
is the case)

[Rodolph Perfetta, 2010-09-27 14:17:23]

http://codereview.chromium.org/3446024/diff/11001/12001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3446024/diff/11001/12001#newcode1699
src/arm/stub-cache-arm.cc:1699: __ sub(r0, r0, r1);
Did you mean sub(r0, r0, r1, SetCC) ?

Also wouldn't the sequence:
  cmp    r0, #0
  rsbmis r0, r0, #0
  bmi    slow
be better for an integer abs (or did I miss a smi corner case?)

http://codereview.chromium.org/3446024/diff/11001/12001#newcode1720
src/arm/stub-cache-arm.cc:1720: __ tst(r1, r3);
merge the mov with the tst: tst(r1, Operand(HeapNumber::kSignMask)).

You will need to do the same for the eor further down.

[Vitaly Repeshko, 2010-09-27 14:56:59]

Rico, thanks for the review!

Rodolph, thanks for the additional comments! Could you please take another look
at the ARM code?


-- Vitaly

http://codereview.chromium.org/3446024/diff/11001/12001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3446024/diff/11001/12001#newcode1699
src/arm/stub-cache-arm.cc:1699: __ sub(r0, r0, r1);
On 2010/09/27 14:17:23, Rodolph Perfetta wrote:
> Did you mean sub(r0, r0, r1, SetCC) ?

Oops, fixed.

> Also wouldn't the sequence:
>   cmp    r0, #0
>   rsbmis r0, r0, #0
>   bmi    slow
> be better for an integer abs (or did I miss a smi corner case?)

I changed the current approach to only use two instructions. You are very much
welcome to send me a patch with your approach if you think it's better.

http://codereview.chromium.org/3446024/diff/11001/12001#newcode1720
src/arm/stub-cache-arm.cc:1720: __ tst(r1, r3);
On 2010/09/27 14:17:23, Rodolph Perfetta wrote:
> merge the mov with the tst: tst(r1, Operand(HeapNumber::kSignMask)).
> 
> You will need to do the same for the eor further down.

Will the constant fit into the instruction? I was afraid of using the constant
pool more than necessary, but maybe it's not an issue.

http://codereview.chromium.org/3446024/diff/11001/12002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/11001/12002#newcode2016
src/ia32/stub-cache-ia32.cc:2016: __ mov(ebx, FieldOperand(eax,
HeapNumber::kExponentOffset));
On 2010/09/27 13:18:23, Rico wrote:
> You could potentially use an xmm register here and do (assume loaded into
xmm0):
> pxor(xmm1, xmm1);
> subsd(xmm1, xmm0)
> and(xmm0, xmm1)

Sure, I could do branchless floating point code as well, but I want to avoid
allocation in case the number is not negative already.

http://codereview.chromium.org/3446024/diff/11001/12006
File test/mjsunit/math-abs.js (right):

http://codereview.chromium.org/3446024/diff/11001/12006#newcode28
test/mjsunit/math-abs.js:28: // Flags: --max-new-space-size=262144
On 2010/09/27 13:18:23, Rico wrote:
> Why this flag?

I want to test GC effects and so want GC to happen earlier.

http://codereview.chromium.org/3446024/diff/11001/12006#newcode78
test/mjsunit/math-abs.js:78: assertEquals(two_30 - 1, Math.abs(-two_30 + 1));
On 2010/09/27 13:18:23, Rico wrote:
> Maybe add a few test cases where you call Math.abs on objects, arrays and
> strings that are not actual numbers (to test that we do indeed bail out if
this
> is the case)

Good idea. Done.

[Rodolph Perfetta, 2010-09-27 17:32:15]

On 2010/09/27 14:56:59, Vitaly wrote:

> http://codereview.chromium.org/3446024/diff/11001/12001#newcode1699
> src/arm/stub-cache-arm.cc:1699: __ sub(r0, r0, r1);
> On 2010/09/27 14:17:23, Rodolph Perfetta wrote:
> 
> > Also wouldn't the sequence:
> >   cmp    r0, #0
> >   rsbmis r0, r0, #0
> >   bmi    slow
> > be better for an integer abs (or did I miss a smi corner case?)
> 
> I changed the current approach to only use two instructions. You are very much
> welcome to send me a patch with your approach if you think it's better.

The rewritten sequence is as good as mine, this is just a matter of personal
preference now.
 
> http://codereview.chromium.org/3446024/diff/11001/12001#newcode1720
> src/arm/stub-cache-arm.cc:1720: __ tst(r1, r3);
> On 2010/09/27 14:17:23, Rodolph Perfetta wrote:
> > merge the mov with the tst: tst(r1, Operand(HeapNumber::kSignMask)).
> > 
> > You will need to do the same for the eor further down.
> 
> Will the constant fit into the instruction? I was afraid of using the constant
> pool more than necessary, but maybe it's not an issue.

V8 assembler is quite smart and can cope with any constant. Obviously constant
out of range will incur more code.

HeapNumber::kSignMask is 0x80000000u and this will fit into ARM in-instructions
constant: 8bits rotated right by an even number of bits between 0 and 30.

[Rodolph Perfetta, 2010-09-27 17:33:49]

http://codereview.chromium.org/3446024/diff/12011/10003
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3446024/diff/12011/10003#newcode1718
src/arm/stub-cache-arm.cc:1718: __ b(nz, &negative_sign);
I know this is a nit picking but nz is not an ARM condition code, ne (not equal)
is. In V8 nz and ne are aliases probably to make it easier to move between x86
and ARM.

[Vitaly Repeshko, 2010-09-27 22:44:00]

Rodolph,

Thanks very much for these explanations! I switched to using ne condition code
and sign mask immediate.


-- Vitaly

[antonm, 2010-09-28 12:04:17]

http://codereview.chromium.org/3446024/diff/22001/23001
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/3446024/diff/22001/23001#newcode1671
src/arm/stub-cache-arm.cc:1671: STATIC_ASSERT(kSmiTag == 0);
BranchOnSmi(r1, miss)?

http://codereview.chromium.org/3446024/diff/22001/23002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/3446024/diff/22001/23002#newcode1970
src/ia32/stub-cache-ia32.cc:1970: __ mov(edx, Operand(esp, 2 * kPointerSize));
should you load receiver into edx?  Maybe check for its smi-ness with an
operand?  You apparently don't need receiver anymore.

http://codereview.chromium.org/3446024/diff/22001/23002#newcode2010
src/ia32/stub-cache-ia32.cc:2010: __ ret(2 * kPointerSize);
Maybe unify this and ARM ret: in ARM code you use argc while here you doesn't.

http://codereview.chromium.org/3446024/diff/22001/23004
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/3446024/diff/22001/23004#newcode1585
src/x64/stub-cache-x64.cc:1585: 
nit: maybe drop blank lines here, we imho don't add to readability

http://codereview.chromium.org/3446024/diff/22001/23004#newcode1603
src/x64/stub-cache-x64.cc:1603: __ SmiToInteger32(rax, rax);
do we need this tagging/untagging?  Looks like MSB of Smi still gives you the
sign and you can proceed exactly like in ia32 case?  (probably adding some
asserts/extending number of tests).

I won't be surprised if it'd be notably faster for fastest case.