Add optimized ICs for new CanvasArray types. This is a follow-on CL to...

Add optimized ICs for new CanvasArray types introduced in WebGL
specification under development. This is a follow-on CL to
http://codereview.chromium.org/293023 .

Based on review feedback, defined the behavior of storing NaN and
+/-Infinity into external arrays of integer types as storing 0. Added
test cases. Added fucomi instruction to assembler. Fixed bug in
KeyedLoadIC::GenerateExternalArray when allocation of HeapNumber
failed. Fixed bug in encoding of 16-bit immediate arithmetic
instructions in 64-bit port.

Removed raising of exceptions for negative array indices passed to
external arrays and associated tests. Based on current discussion in
WebGL working group, will probably end up removing the exception
throwing altogether.


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

[Ken Russell (switch to Gerrit), 2009-10-20 15:49:54]

Please review. Thanks.

[William Hesse, 2009-10-21 08:55:20]

LGTM.

http://codereview.chromium.org/294022/diff/1002/10
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/294022/diff/1002/10#newcode394
Line 394: __ j(above_equal, &slow);
// go to slow if key is negative or ge length.

http://codereview.chromium.org/294022/diff/1002/10#newcode441
Line 441: __ j(above_equal, &box_int);
I think these four lines are equivalent to 
__cmp(eax, 0xC0000000)
__j(sign, &box_int)

If they are copied from somewhere else, that should be changed too.

http://codereview.chromium.org/294022/diff/1002/10#newcode688
Line 688: (array_type == kExternalFloatArray) ? taken : not_taken);
Hints are disabled by default, and we should probably get rid of them
altogether, so you don't need to do them.

http://codereview.chromium.org/294022/diff/1002/11
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/294022/diff/1002/11#newcode404
Line 404: __ j(above_equal, &slow);
Shouldn't this be a signed comparison?  I think we use an unsigned comparison in
other keyed ICs because we have earlier checked both the smi tag and the sign
bit of the key, leaving only positive smis.

Oh, nevermind - it is putting the negative ones up, so they are caught too. 
Comment this: "Unsigned comparison catches both negative values and values that
are too big."

http://codereview.chromium.org/294022/diff/1002/11#newcode696
Line 696: __ movq(rdx, Operand(rsp, 2 * kPointerSize));  // 2 ~ return address,
key
This comment is from a long time ago, and is confusing.  Now that we have the
----- S t a t e -----  header, it can be removed.

http://codereview.chromium.org/294022/diff/1002/11#newcode782
Line 782: __ fistp_d(Operand(rsp, 0));
Does this do the right thing for NaN and infinities?

[Christian Plesner Hansen, 2009-10-21 09:18:40]

Most comments from ic-ia32 also apply to ic-x64.

http://codereview.chromium.org/294022/diff/1002/10
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/294022/diff/1002/10#newcode371
Line 371: __ movzx_b(ebx, FieldOperand(edx, Map::kBitFieldOffset));
Depending on how much performance this costs we could alternatively forbid
access checks on objects with external array elements.

http://codereview.chromium.org/294022/diff/1002/10#newcode394
Line 394: __ j(above_equal, &slow);
What about negative indices?

http://codereview.chromium.org/294022/diff/1002/10#newcode421
Line 421: ASSERT(false);
UNREACHABLE()

http://codereview.chromium.org/294022/diff/1002/10#newcode443
Line 443: ASSERT(array_type == kExternalUnsignedIntArray);
ASSERT_EQ

http://codereview.chromium.org/294022/diff/1002/10#newcode679
Line 679: __ j(above_equal, &slow);
Negative?

http://codereview.chromium.org/294022/diff/1002/10#newcode726
Line 726: __ fld_d(FieldOperand(eax, HeapNumber::kValueOffset));
Please add a comment about how this handles NaN, Infinity and other odd values.

[Ken Russell (switch to Gerrit), 2009-10-21 11:10:28]

http://codereview.chromium.org/294022/diff/1002/10
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/294022/diff/1002/10#newcode394
Line 394: __ j(above_equal, &slow);
On 2009/10/21 08:55:20, William Hesse wrote:
> // go to slow if key is negative or ge length.

Done.

http://codereview.chromium.org/294022/diff/1002/10#newcode394
Line 394: __ j(above_equal, &slow);
On 2009/10/21 09:18:40, Christian Plesner Hansen wrote:
> What about negative indices?

They are handled via the unsigned (above_equal) comparison -- we go to the slow
case.

http://codereview.chromium.org/294022/diff/1002/10#newcode421
Line 421: ASSERT(false);
On 2009/10/21 09:18:40, Christian Plesner Hansen wrote:
> UNREACHABLE()

Done.

http://codereview.chromium.org/294022/diff/1002/10#newcode441
Line 441: __ j(above_equal, &box_int);
On 2009/10/21 08:55:20, William Hesse wrote:
> I think these four lines are equivalent to 
> __cmp(eax, 0xC0000000)
> __j(sign, &box_int)
> 
> If they are copied from somewhere else, that should be changed too.

Per the comment, this is the assembly translation of Smi::IsValid. Since I know
this instruction sequence works I'm going to leave it as is.

http://codereview.chromium.org/294022/diff/1002/10#newcode443
Line 443: ASSERT(array_type == kExternalUnsignedIntArray);
On 2009/10/21 09:18:40, Christian Plesner Hansen wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/294022/diff/1002/10#newcode679
Line 679: __ j(above_equal, &slow);
On 2009/10/21 09:18:40, Christian Plesner Hansen wrote:
> Negative?

Handled by this comparison. See above.

http://codereview.chromium.org/294022/diff/1002/10#newcode688
Line 688: (array_type == kExternalFloatArray) ? taken : not_taken);
On 2009/10/21 08:55:20, William Hesse wrote:
> Hints are disabled by default, and we should probably get rid of them
> altogether, so you don't need to do them.

OK. They aren't present in the 64-bit port anyway.

http://codereview.chromium.org/294022/diff/1002/10#newcode726
Line 726: __ fld_d(FieldOperand(eax, HeapNumber::kValueOffset));
On 2009/10/21 09:18:40, Christian Plesner Hansen wrote:
> Please add a comment about how this handles NaN, Infinity and other odd
values.

Done. Their cast value is not specified.

http://codereview.chromium.org/294022/diff/1002/11
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/294022/diff/1002/11#newcode404
Line 404: __ j(above_equal, &slow);
On 2009/10/21 08:55:20, William Hesse wrote:
> Shouldn't this be a signed comparison?  I think we use an unsigned comparison
in
> other keyed ICs because we have earlier checked both the smi tag and the sign
> bit of the key, leaving only positive smis.
> 
> Oh, nevermind - it is putting the negative ones up, so they are caught too. 
> Comment this: "Unsigned comparison catches both negative values and values
that
> are too big."

Done.

http://codereview.chromium.org/294022/diff/1002/11#newcode696
Line 696: __ movq(rdx, Operand(rsp, 2 * kPointerSize));  // 2 ~ return address,
key
On 2009/10/21 08:55:20, William Hesse wrote:
> This comment is from a long time ago, and is confusing.  Now that we have the
> ----- S t a t e -----  header, it can be removed.

Done.

http://codereview.chromium.org/294022/diff/1002/11#newcode782
Line 782: __ fistp_d(Operand(rsp, 0));
On 2009/10/21 08:55:20, William Hesse wrote:
> Does this do the right thing for NaN and infinities?

These arrays don't specify what happens for those values, so the error behavior
should be acceptable.

[Christian Plesner Hansen, 2009-10-21 11:49:57]

http://codereview.chromium.org/294022/diff/5001/5002
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/294022/diff/5001/5002#newcode731
Line 731: // essentially store as unspecified values.
Does that mean we'll get nondeterministic behavior when writing and reading back
a NaN?  That would be Bad(TM).

[Ken Russell (switch to Gerrit), 2009-10-22 13:45:42]

http://codereview.chromium.org/294022/diff/5001/5002
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/294022/diff/5001/5002#newcode731
Line 731: // essentially store as unspecified values.
On 2009/10/21 11:49:57, Christian Plesner Hansen wrote:
> Does that mean we'll get nondeterministic behavior when writing and reading
back
> a NaN?  That would be Bad(TM).

You're right. I've modified the code to store NaN and +/-Infinity into external
integer array types as 0. Please re-review.

[Christian Plesner Hansen, 2009-10-22 14:14:29]

Stv!

[William Hesse, 2009-10-22 14:45:39]

LGTM.  I will add fucomi to the disassembler.