Issue 6287030: Create specialized code stubs for PixelArray loads.

Issue 6287030: Create specialized code stubs for PixelArray loads. (Closed)

Created:
9 years, 10 months ago by danno

Modified:
9 years, 7 months ago

Reviewers:
Søren Thygesen Gjesse, Mads Ager (chromium)

CC:
Bernhard Bauer

Base URL:
https://v8.googlecode.com/svn/trunk

Visibility:
Public.

Description

Create specialized code stubs for PixelArray loads.

Patch Set 2 : cleanup before review #

Total comments: 39

Patch Set 3 : review feedback #

Created: 9 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+510 lines, -59 lines)			Patch
M	src/arm/code-stubs-arm.h	View	1 2	1 chunk	+21 lines, -0 lines	0 comments	Download
M	src/arm/code-stubs-arm.cc	View	1 2	1 chunk	+67 lines, -0 lines	0 comments	Download
M	src/arm/ic-arm.cc	View	1	1 chunk	+12 lines, -13 lines	0 comments	Download
M	src/arm/stub-cache-arm.cc	View	1 2	1 chunk	+32 lines, -0 lines	0 comments	Download
M	src/heap.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/ia32/code-stubs-ia32.h	View	1 2	1 chunk	+19 lines, -0 lines	0 comments	Download
M	src/ia32/code-stubs-ia32.cc	View	1 2	1 chunk	+48 lines, -0 lines	0 comments	Download
M	src/ia32/ic-ia32.cc	View	1	1 chunk	+9 lines, -13 lines	0 comments	Download
M	src/ia32/macro-assembler-ia32.h	View	1 2	1 chunk	+11 lines, -0 lines	0 comments	Download
M	src/ia32/stub-cache-ia32.cc	View	1 2	1 chunk	+31 lines, -0 lines	0 comments	Download
M	src/ic.cc	View	1 2	1 chunk	+25 lines, -17 lines	0 comments	Download
M	src/runtime.cc	View	2	1 chunk	+4 lines, -2 lines	0 comments	Download
M	src/stub-cache.h	View		2 chunks	+4 lines, -0 lines	0 comments	Download
M	src/stub-cache.cc	View	1 2	2 chunks	+33 lines, -0 lines	0 comments	Download
M	src/x64/code-stubs-x64.h	View	1 2	1 chunk	+19 lines, -0 lines	0 comments	Download
M	src/x64/code-stubs-x64.cc	View	1 2	1 chunk	+47 lines, -0 lines	0 comments	Download
M	src/x64/ic-x64.cc	View	1	1 chunk	+9 lines, -14 lines	0 comments	Download
M	src/x64/stub-cache-x64.cc	View	1 2	1 chunk	+29 lines, -0 lines	0 comments	Download
M	test/cctest/test-api.cc	View	1 2	1 chunk	+87 lines, -0 lines	0 comments	Download
M	test/mjsunit/regress/regress-900966.js	View		1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

danno

Please take a look again, I have reworked this pixel array ICs to be map-specific ...

9 years, 10 months ago (2011-02-01 11:36:40 UTC) #1

Mads Ager (chromium)

Let's use the CheckMap macro-assembler call more. When that is done this looks good. http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc ...

9 years, 10 months ago (2011-02-01 12:02:12 UTC) #2

Søren Thygesen Gjesse

LGTM http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc#newcode5720 src/arm/code-stubs-arm.cc:5720: Please add a comment and mention how the ...

9 years, 10 months ago (2011-02-01 12:02:56 UTC) #3

danno

9 years, 10 months ago (2011-02-03 12:53:26 UTC) #4

Feedback addressed and committed.

http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:5720: 
On 2011/02/01 12:02:56, Søren Gjesse wrote:
> Please add a comment and mention how the different exit lables work (maybe in
> the .h file).

Done.

http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:5758: __ LoadRoot(scratch1,
Heap::kPixelArrayMapRootIndex);
On 2011/02/01 12:02:12, Mads Ager wrote:
> Can you use __ CheckMap() that accepts a root array index?

Done.

http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:5764: if (key_not_smi != NULL) {
On 2011/02/01 12:02:56, Søren Gjesse wrote:
> Please explain the if here. If key_not_smi is NULL key needs to have been smi
> checked already - right? And if that is the case please add else part
> 
> } else {
>   if (FLAG_debug_code) {
>     __ ABortIfNotSmi(key);
>   }
> }

Done.

http://codereview.chromium.org/6287030/diff/2001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:5770: __ b(hs, out_of_range);
On 2011/02/01 12:02:56, Søren Gjesse wrote:
> Maybe comment that the unsigned check covers negative keys.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:3099: __ tst(r1, Operand(kSmiTagMask));
On 2011/02/01 12:02:12, Mads Ager wrote:
> JumpIfSmi?

Done.

http://codereview.chromium.org/6287030/diff/2001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:3103: __ ldr(r2, FieldMemOperand(r1,
HeapObject::kMapOffset));
On 2011/02/01 12:02:12, Mads Ager wrote:
> CheckMap? I think you can even have check map include both the smi and the map
> check.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:6530: if (key_not_smi != NULL) {
On 2011/02/01 12:02:56, Søren Gjesse wrote:
> See comments for ARM. Also on ARM you check the receiver before the key, but
of
> cause that does not matter.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:6537: // Verify that the receiver has pixel array
elements.
On 2011/02/01 12:02:12, Mads Ager wrote:
> Perform checks in the same order on all platforms for consistency? On ARM the
> pixel array check is first.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3171: __ cmp(FieldOperand(edx,
HeapObject::kMapOffset),
On 2011/02/01 12:02:12, Mads Ager wrote:
> CheckMap? I believe CheckMap can check for smi as well.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/ic.cc#newcode1214
src/ic.cc:1214: StubCache::ComputeKeyedLoadOrStoreExternalArray(*receiver,
false);
On 2011/02/01 12:02:12, Mads Ager wrote:
> Four-space indent. Which will probably lead to a long line so you have to have
> one argument per line as well. Urgh.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ic.cc#newcode1221
src/ic.cc:1221: StubCache::ComputeKeyedLoadPixelArray(*receiver);
On 2011/02/01 12:02:12, Mads Ager wrote:
> Four-space indent.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ic.cc#newcode1223
src/ic.cc:1223: probe->IsFailure() ? NULL :
Code::cast(probe->ToObjectUnchecked());
On 2011/02/01 12:02:12, Mads Ager wrote:
> Four-space indent.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ic.cc#newcode1227
src/ic.cc:1227: StubCache::ComputeKeyedLoadSpecialized(*receiver);
On 2011/02/01 12:02:12, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/ic.cc#newcode1229
src/ic.cc:1229: probe->IsFailure() ? NULL :
Code::cast(probe->ToObjectUnchecked());
On 2011/02/01 12:02:12, Mads Ager wrote:
> Reindent. We need four-space indent so in this case we should split at the ?:.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/runtime.cc#newcode3494
src/runtime.cc:3494: if (index >= 0 && index < str->length()) {
On 2011/02/01 12:02:56, Søren Gjesse wrote:
> Is this a bug-fix? If so maybe there should be a regression test.

There's actually an existing test case that wasn't quite sufficient to cause
this problem to show up in debug mode (although it did in release!) since it
didn't call a function a sufficient number of times to cause all of the map IC
transitions to land in the generic keyed load IC: regress-900966.js. I augmented
the test case to make sure it covers all of the transitions.

http://codereview.chromium.org/6287030/diff/2001/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/stub-cache.cc#newcode466
src/stub-cache.cc:466: Code::ComputeMonomorphicFlags(Code::KEYED_LOAD_IC,
NORMAL);
On 2011/02/01 12:02:12, Mads Ager wrote:
> This is sort of a misuse of the NORMAL tag which normally means that the
object
> is slow-case with respect to the properties array. Since this is working on
the
> elements part I understand how it works. Maybe we can add a comment here and
in
> LoadSpecialized above about why NORMAL works as a tag here?

Done.

http://codereview.chromium.org/6287030/diff/2001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4437: if (key_not_smi != NULL) {
On 2011/02/01 12:02:56, Søren Gjesse wrote:
> See ARM comments.

Done.

http://codereview.chromium.org/6287030/diff/2001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:4443: __ movq(elements, FieldOperand(receiver,
JSObject::kElementsOffset));
On 2011/02/01 12:02:12, Mads Ager wrote:
> Same order of checks on all platforms? Use CheckMap?

Done.

http://codereview.chromium.org/6287030/diff/2001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/6287030/diff/2001/src/x64/stub-cache-x64.cc#ne...
src/x64/stub-cache-x64.cc:3013: __ Cmp(FieldOperand(rdx,
HeapObject::kMapOffset),
On 2011/02/01 12:02:12, Mads Ager wrote:
> Use CheckMap to check both map and not smi?

Done.

Expand Messages | Collapse Messages