Add custom typed ICs for pixel array loads.

Add custom typed ICs for pixel array loads. 

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

[danno, 2011-01-25 12:05:31]

please review

[Mads Ager (chromium), 2011-01-25 14:49:17]

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc#newcode1395
src/arm/ic-arm.cc:1395: __ ldr(elements, FieldMemOperand(r1,
JSObject::kElementsOffset));
We need to check that the receiver is a JSObject before we can load the
elements. When the object is not a smi it has to be a heap object. However,
there are heap objects that have no elements array (such as value objects and
undefined and null).

I think you need to use GenerateKeyedReceiverCheck or something like that. We
could also get an object that requires access checks or has a keyed interceptor.

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc#newcode1403
src/arm/ic-arm.cc:1403: __ ldr(ip, FieldMemOperand(elements,
PixelArray::kLengthOffset));
Since we have enough register here, I would prefer not to use ip. Using ip
always makes me a bit nervous because it is occasionally clobered by the
macroassembler.

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc#newcode1409
src/arm/ic-arm.cc:1409: __ ldr(ip, FieldMemOperand(elements,
PixelArray::kExternalPointerOffset));
Ditto for using ip.

http://codereview.chromium.org/6323002/diff/40001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/ia32/ic-ia32.cc#newcode775
src/ia32/ic-ia32.cc:775: // Check that the key is an array index, that is
Uint32.
This code looks more complicated than on ARM and IA32. Not sure I understand why
this code would be faster than using basically the same code as on x64?

http://codereview.chromium.org/6323002/diff/40001/src/ia32/ic-ia32.cc#newcode787
src/ia32/ic-ia32.cc:787: __ mov(ecx, FieldOperand(edx,
JSObject::kElementsOffset));
Need a JSObject check.

http://codereview.chromium.org/6323002/diff/40001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/ic.cc#newcode1202
src/ic.cc:1202: stub = pixel_array_stub();
Let's remove the pixel array code in the generic stub now that we have a special
IC stub?

http://codereview.chromium.org/6323002/diff/40001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/x64/ic-x64.cc#newcode780
src/x64/ic-x64.cc:780: __ movq(rcx, FieldOperand(rdx,
JSObject::kElementsOffset));
JSObject check needed.

[danno, 2011-01-25 20:56:29]

addressed review feedback

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc#newcode1395
src/arm/ic-arm.cc:1395: __ ldr(elements, FieldMemOperand(r1,
JSObject::kElementsOffset));
On 2011/01/25 14:49:17, Mads Ager wrote:
> We need to check that the receiver is a JSObject before we can load the
> elements. When the object is not a smi it has to be a heap object. However,
> there are heap objects that have no elements array (such as value objects and
> undefined and null).
> 
> I think you need to use GenerateKeyedReceiverCheck or something like that. We
> could also get an object that requires access checks or has a keyed
interceptor.

Done.

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc#newcode1403
src/arm/ic-arm.cc:1403: __ ldr(ip, FieldMemOperand(elements,
PixelArray::kLengthOffset));
On 2011/01/25 14:49:17, Mads Ager wrote:
> Since we have enough register here, I would prefer not to use ip. Using ip
> always makes me a bit nervous because it is occasionally clobered by the
> macroassembler.

Done.

http://codereview.chromium.org/6323002/diff/40001/src/arm/ic-arm.cc#newcode1409
src/arm/ic-arm.cc:1409: __ ldr(ip, FieldMemOperand(elements,
PixelArray::kExternalPointerOffset));
On 2011/01/25 14:49:17, Mads Ager wrote:
> Ditto for using ip.

Done.

http://codereview.chromium.org/6323002/diff/40001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/ia32/ic-ia32.cc#newcode775
src/ia32/ic-ia32.cc:775: // Check that the key is an array index, that is
Uint32.
On 2011/01/25 14:49:17, Mads Ager wrote:
> This code looks more complicated than on ARM and IA32. Not sure I understand
why
> this code would be faster than using basically the same code as on x64?

Done.

http://codereview.chromium.org/6323002/diff/40001/src/ia32/ic-ia32.cc#newcode787
src/ia32/ic-ia32.cc:787: __ mov(ecx, FieldOperand(edx,
JSObject::kElementsOffset));
On 2011/01/25 14:49:17, Mads Ager wrote:
> Need a JSObject check.

Done.

http://codereview.chromium.org/6323002/diff/40001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/ic.cc#newcode1202
src/ic.cc:1202: stub = pixel_array_stub();
This occurred to me too, but I actually think we still need it... Otherwise how
do we handle pixel arrays in the case of a call site that goes megamorphic?
On 2011/01/25 14:49:17, Mads Ager wrote:
> Let's remove the pixel array code in the generic stub now that we have a
special
> IC stub?

http://codereview.chromium.org/6323002/diff/40001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/x64/ic-x64.cc#newcode780
src/x64/ic-x64.cc:780: __ movq(rcx, FieldOperand(rdx,
JSObject::kElementsOffset));
On 2011/01/25 14:49:17, Mads Ager wrote:
> JSObject check needed.

Done.

[Mads Ager (chromium), 2011-01-26 08:10:20]

LGTM, with one suggestion.

http://codereview.chromium.org/6323002/diff/40001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6323002/diff/40001/src/ic.cc#newcode1202
src/ic.cc:1202: stub = pixel_array_stub();
True, we don't deal with it in that case and I guess it will not harm much to
leave it in the generic stub. 

However, can we share some of the code generation code in that case? Create a
static helper method GenerateLoadFromPixelArray that generates the code for
everything but the receiver check?

On 2011/01/25 20:56:29, danno wrote:
> This occurred to me too, but I actually think we still need it... Otherwise
how
> do we handle pixel arrays in the case of a call site that goes megamorphic?
> On 2011/01/25 14:49:17, Mads Ager wrote:
> > Let's remove the pixel array code in the generic stub now that we have a
> special
> > IC stub?
>

[Søren Thygesen Gjesse, 2011-01-26 21:35:17]

Drive by comments.

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc#newcode1408
src/arm/ic-arm.cc:1408: __ BranchOnNotSmi(key, &slow);
This has changed to JumpIfNotSmi.

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc#newcode1410
src/arm/ic-arm.cc:1410: __ mov(index, Operand(key, ASR, kSmiTagSize));  // Untag
index
There is a SmiUntag instruction in the macro assembler which takes two
registres.

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc#newcode1418
src/arm/ic-arm.cc:1418: __ mov(r0, Operand(scratch2, LSL, kSmiTagSize));  // Tag
result as smi.
There is also a SmiTag which takes two registers, but you could just load it
into r0 and use SmiTag on one register.

[danno, 2011-01-28 10:32:08]

addressed feedback, added test cases to exercise IC code paths.

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc#newcode1408
src/arm/ic-arm.cc:1408: __ BranchOnNotSmi(key, &slow);
On 2011/01/26 21:35:17, Søren Gjesse wrote:
> This has changed to JumpIfNotSmi.

Done.

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc#newcode1410
src/arm/ic-arm.cc:1410: __ mov(index, Operand(key, ASR, kSmiTagSize));  // Untag
index
On 2011/01/26 21:35:17, Søren Gjesse wrote:
> There is a SmiUntag instruction in the macro assembler which takes two
> registres.

Done.

http://codereview.chromium.org/6323002/diff/28003/src/arm/ic-arm.cc#newcode1418
src/arm/ic-arm.cc:1418: __ mov(r0, Operand(scratch2, LSL, kSmiTagSize));  // Tag
result as smi.
On 2011/01/26 21:35:17, Søren Gjesse wrote:
> There is also a SmiTag which takes two registers, but you could just load it
> into r0 and use SmiTag on one register.

Done.

[danno, 2011-01-28 10:35:36]

http://codereview.chromium.org/6323002/diff/74001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6323002/diff/74001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:10178: // CHECK_EQ(1, result->Int32Value());
i'll fix this, shouldn't be commented out.

http://codereview.chromium.org/6323002/diff/74001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:10356: "for (var i = 0; i < 10; ++i) { result =
pa_load(just_ints); }"
I'll fix the 80 cols, too, here and below.

[Mads Ager (chromium), 2011-01-28 10:39:16]

LGTM

http://codereview.chromium.org/6323002/diff/74001/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6323002/diff/74001/src/arm/ic-arm.cc#newcode548
src/arm/ic-arm.cc:548: __ JumpIfNotSmi(key, key_not_smi);
Either make the entire 'if (...) __ JumpIfNotSmi(...);' a one-liner or use
braces around the body.

http://codereview.chromium.org/6323002/diff/74001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/6323002/diff/74001/src/ia32/ic-ia32.cc#newcode513
src/ia32/ic-ia32.cc:513: // Key must be a smi..
.. -> .

http://codereview.chromium.org/6323002/diff/74001/src/ia32/ic-ia32.cc#newcode526
src/ia32/ic-ia32.cc:526: Label oor;
Unused label?

http://codereview.chromium.org/6323002/diff/74001/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/6323002/diff/74001/src/x64/ic-x64.cc#newcode538
src/x64/ic-x64.cc:538: if (key_not_smi != NULL)
Braces or one-liner.

http://codereview.chromium.org/6323002/diff/74001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6323002/diff/74001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:10178: // CHECK_EQ(1, result->Int32Value());
On 2011/01/28 10:35:36, danno wrote:
> i'll fix this, shouldn't be commented out.

Thanks!

http://codereview.chromium.org/6323002/diff/74001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:10356: "for (var i = 0; i < 10; ++i) { result =
pa_load(just_ints); }"
On 2011/01/28 10:35:36, danno wrote:
> I'll fix the 80 cols, too, here and below.

Thanks.