Make sure transitioned arrays efficiently call builtin Array functions

Make sure transitioned arrays efficiently call builtin Array functions

Loosen the requirement for Map equivalency on several map checks, including checks up the prototype chain, that are not sensitive to ElementsKinds. These selected map checks should also match against FAST_DOUBLE_ELEMENT and FAST_ELEMENT transitions of the original map. This specifically helps all variants of transitioned JSArrays to still efficiently call builtins like push, pop and sort.

BUG=none
TEST=none

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

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

[danno, 2012-01-03 16:01:08]

PTAL

[fschneider, 2012-01-04 09:20:18]

Most plaform-specific feedback applies to the other platforms as well.

http://codereview.chromium.org/9015020/diff/7001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/9015020/diff/7001/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:1: // Copyright 2012 the V8 project authors. All
rights reserved.
There seems to be no other change to this file?

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

http://codereview.chromium.org/9015020/diff/7001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:882: __ CheckMap(reg, current_map, miss,
DONT_DO_SMI_CHECK,
Is this part of the change missing in stub-cache-arm.cc?

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3758: Map* transitioned_double_map =
Handle<Map> transitioned_double_map(...)

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3758: Map* transitioned_double_map =
I'd order this differently and move this down closer to the if-statement where
the result is acutally used.

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3762: Map* transitioned_fast_element_map =
Handle<Map> transitioned_fast_element_map(...)

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3787: Handle<Map> map =
Handle<Map>(instr->hydrogen()->map());
Simpler would be:

Handle<Map> map = instr->hydrogen()->map();

http://codereview.chromium.org/9015020/diff/10001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/9015020/diff/10001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:2753: if (mode == ALLOW_ELEMENT_TRANSITION_MAPS)
{
The following code is the same as in lithium-codegen, except there is a branch
at the end instead of the deoptimize. I'd strongly prefer having only one
version of this code around.

[danno, 2012-01-04 10:42:15]

Feedback addressed, please take another look.

http://codereview.chromium.org/9015020/diff/7001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/9015020/diff/7001/src/hydrogen-instructions.cc...
src/hydrogen-instructions.cc:1: // Copyright 2012 the V8 project authors. All
rights reserved.
On 2012/01/04 09:20:18, fschneider wrote:
> There seems to be no other change to this file?

Done.

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

http://codereview.chromium.org/9015020/diff/7001/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:882: __ CheckMap(reg, current_map, miss,
DONT_DO_SMI_CHECK,
On 2012/01/04 09:20:18, fschneider wrote:
> Is this part of the change missing in stub-cache-arm.cc?

Done.

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3758: Map* transitioned_double_map =
On 2012/01/04 09:20:18, fschneider wrote:
> I'd order this differently and move this down closer to the if-statement where
> the result is acutally used.

Done.

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3758: Map* transitioned_double_map =
On 2012/01/04 09:20:18, fschneider wrote:
> Handle<Map> transitioned_double_map(...)

Done.

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3762: Map* transitioned_fast_element_map =
On 2012/01/04 09:20:18, fschneider wrote:
> Handle<Map> transitioned_fast_element_map(...)

Done.

http://codereview.chromium.org/9015020/diff/10001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:3787: Handle<Map> map =
Handle<Map>(instr->hydrogen()->map());
On 2012/01/04 09:20:18, fschneider wrote:
> Simpler would be:
> 
> Handle<Map> map = instr->hydrogen()->map();

Done.

http://codereview.chromium.org/9015020/diff/10001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/9015020/diff/10001/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:2753: if (mode == ALLOW_ELEMENT_TRANSITION_MAPS)
{
On 2012/01/04 09:20:18, fschneider wrote:
> The following code is the same as in lithium-codegen, except there is a branch
> at the end instead of the deoptimize. I'd strongly prefer having only one
> version of this code around.

Done.

[fschneider, 2012-01-04 12:17:51]

lgtm

http://codereview.chromium.org/9015020/diff/14002/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/9015020/diff/14002/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:2004: Map* transitioned_fast_element_map(
Handle<Map>

http://codereview.chromium.org/9015020/diff/14002/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:2013: Map* transitioned_double_map(
Handle<Map>

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:497: Map* transitioned_fast_element_map(
If there is no specific reason to use a raw pointer, I'd replace Map* with
Handle<Map> here.

If code gets moved or copied around, there may be a danger of using raw pointers
where we must not.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:504:
Handle<Map>(transitioned_fast_element_map));
Then there is no need to create a handle here.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:507: Map* transitioned_double_map(
Same here.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:514: Handle<Map>(transitioned_double_map));
and here.

http://codereview.chromium.org/9015020/diff/14002/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/9015020/diff/14002/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:2749: Map* transitioned_fast_element_map(
Handle<Map>

http://codereview.chromium.org/9015020/diff/14002/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:2759: Map* transitioned_double_map(
Handle<Map>

[danno, 2012-01-05 13:41:57]

Please take another look

http://codereview.chromium.org/9015020/diff/14002/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/9015020/diff/14002/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:2004: Map* transitioned_fast_element_map(
On 2012/01/04 12:17:51, fschneider wrote:
> Handle<Map>

Done.

http://codereview.chromium.org/9015020/diff/14002/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:2013: Map* transitioned_double_map(
On 2012/01/04 12:17:51, fschneider wrote:
> Handle<Map>

Done.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:497: Map* transitioned_fast_element_map(
On 2012/01/04 12:17:51, fschneider wrote:
> If there is no specific reason to use a raw pointer, I'd replace Map* with
> Handle<Map> here.
> 
> If code gets moved or copied around, there may be a danger of using raw
pointers
> where we must not.

Done.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:504:
Handle<Map>(transitioned_fast_element_map));
On 2012/01/04 12:17:51, fschneider wrote:
> Then there is no need to create a handle here.

Done.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:507: Map* transitioned_double_map(
On 2012/01/04 12:17:51, fschneider wrote:
> Same here.

Done.

http://codereview.chromium.org/9015020/diff/14002/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:514: Handle<Map>(transitioned_double_map));
On 2012/01/04 12:17:51, fschneider wrote:
> and here.

Done.

http://codereview.chromium.org/9015020/diff/14002/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/9015020/diff/14002/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:2749: Map* transitioned_fast_element_map(
On 2012/01/04 12:17:51, fschneider wrote:
> Handle<Map>

Done.

http://codereview.chromium.org/9015020/diff/14002/src/x64/macro-assembler-x64...
src/x64/macro-assembler-x64.cc:2759: Map* transitioned_double_map(
On 2012/01/04 12:17:51, fschneider wrote:
> Handle<Map>

Done.

[fschneider, 2012-01-05 17:15:10]

LGTM.

http://codereview.chromium.org/9015020/diff/27001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/9015020/diff/27001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:1964: bool has_element_transitions_;
This will make each HCheckMap one word larger. Consider packing mode_ and
has_element_transitions_ one bitfield, since they are both only 1-bit values.