Support array literals with FAST_DOUBLE_ELEMENTS ElementsKind.

Support array literals with FAST_DOUBLE_ELEMENTS ElementsKind. 

BUG=none
TEST=test/mjsunit/array-literal.js

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

[danno, 2011-10-18 12:37:57]

PTAL

Guidance:
* This CL allows array literals to correctly be initialized with
FAST_SMI_ONLY_ELEMENTS and FAST_DOUBLE_ELEMENTS in all cases under the
smi-only-arrays flag. The primary changes are in runtime.cc and parser.cc, which
now track the ElementsKind of an array literal. Array literals must now track
the ElementsKind in addition to the FixedArray containing its contant values.
* The full code generator now generates array literal initializers for
dynamically-assigned values that handle element transitions correctly
(non-transitions are handled inline and transitions use the runtime's
SetProperty).
* FastCloneShallowArrayStub (code-stubs-XXX), which is used by both Crankshaft
and the full code generator to materialize literals from boilerplate, has been
augmented to handle FixedDoubleArrays.

[Jakob Kummerow, 2011-10-18 15:42:36]

LGTM with nits.

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1549: // Do tail-call to runtime routine.
This is not a tail call.

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1553: // Array literal has ElementsKind of
FAST_DOUBLE_ELEMENTS
nit: missing dot at the end.

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1563: r7,
Urgh. This wastes vertical space and does not increase readability IMHO.
Suggestion:

__ StoreNumberToDoubleElements(
result_register(), r3, r6, r1, r4, r5, r9, r7, &slow_elements);

Personally, I'm also fine with arbitrary groupings of parameters per line.

http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1528: // Do tail-call to runtime routine.
This is not a tail call.

http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1532: // Array literal has ElementsKind of
FAST_DOUBLE_ELEMENTS
nit: missing dot at the end.

http://codereview.chromium.org/8258015/diff/9001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/parser.cc#newcode3315
src/parser.cc:3315: // ElementsKind.  Start with FAST_SMI_ONLY_ELEMENTS, and
transitions to
nit: s/transitions/transition/

http://codereview.chromium.org/8258015/diff/9001/src/parser.cc#newcode3331
src/parser.cc:3331: for (int j = 0; j < i; ++j) {
This code works, but it's brittle, as it subtly relies on the "if (elements_kind
== FAST_DOUBLE_ELEMENTS)" block being executed after the transition, which might
bite anyone who tries to refactor this. Please do either of these two:
(1) Put in an explicit store of boilerplate_value->Number into the newly
allocated double array (and, at your option, insert an "else" below to avoid
duplication of the store), or
(2) Add a comment explaining the fall-through situation.

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode447
src/runtime.cc:447: // If the ElementKinds of the constant values of the array
literal are less
nit: s/ElementKinds/ElementsKind/, s/are/is/

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode448
src/runtime.cc:448: // specific that the ElementsKind of the boilerplate array
object, change the
nit: s/that/than/

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode452
src/runtime.cc:452: Handle<Map> smi_array_map =
isolate->factory()->GetElementsTransitionMap(
nit: rename smi_array_map to something more generic (transitioned_array_map?
new_array_map?), as it might well be a non-smi map.

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

http://codereview.chromium.org/8258015/diff/9001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:291: // Copy the elements array.
Duplicate line.

http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:1497: // Do tail-call to runtime routine.
This is not a tail call.

http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:1501: // Array literal has ElementsKind of
FAST_DOUBLE_ELEMENTS
nit: missing dot at the end.

http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-t...
File test/mjsunit/array-literal-transitions.js (right):

http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-t...
test/mjsunit/array-literal-transitions.js:128: function d() {
Isn't everything from here on down contained in test_large_literal() anyway?

[Kevin Millikin (Chromium), 2011-10-19 08:10:45]

Comments.

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1534: __ CheckFastElements(r2, r3,
&double_elements);
This code checks the element kind for every stored computed value.  A small
improvement to this code is to check up front, save the kind (tagged in the
stack?) as a state, and update it when a transition is taken.

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1538: __ CheckFastSmiOnlyElements(r2, r3,
&fast_elements);
It's a bit confusing (to me) that the label is for *not* fast smi only elements.
 That's because it seems more natural, given the function name, to interpret it
as something like "branch on fast smi only elements".

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1550: __ CallRuntime(Runtime::kSetProperty, 5);
Save this for a future CL, but I think that it would be better to have a custom
function for exactly setting an element as part of initializing an array
literal.

The general SetProperty has a lot of stuff that we know we don't need (checking
attributes, strict vs. non-strict differences, proxies, etc.).

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1556: __
StoreNumberToDoubleElements(result_register(),
My biggest concern is that this is a lot of generated code (for the double store
particularly, but also the write barrier and all the rest) per-computed value.

It might be better to pass the value, array, index, and current element kind to
a stub (and get back a new element kind or update it in place), where the stub
is a little interpreter for array stores implemented in assembly.

What do you think?

[danno, 2011-10-19 11:36:39]

feedback addressed, landing.

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1549: // Do tail-call to runtime routine.
On 2011/10/18 15:42:36, Jakob wrote:
> This is not a tail call.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1553: // Array literal has ElementsKind of
FAST_DOUBLE_ELEMENTS
On 2011/10/18 15:42:36, Jakob wrote:
> nit: missing dot at the end.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1563: r7,
On 2011/10/18 15:42:36, Jakob wrote:
> Urgh. This wastes vertical space and does not increase readability IMHO.
> Suggestion:
> 
> __ StoreNumberToDoubleElements(
> result_register(), r3, r6, r1, r4, r5, r9, r7, &slow_elements);
> 
> Personally, I'm also fine with arbitrary groupings of parameters per line.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1528: // Do tail-call to runtime routine.
On 2011/10/18 15:42:36, Jakob wrote:
> This is not a tail call.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1532: // Array literal has ElementsKind of
FAST_DOUBLE_ELEMENTS
On 2011/10/18 15:42:36, Jakob wrote:
> nit: missing dot at the end.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/parser.cc#newcode3315
src/parser.cc:3315: // ElementsKind.  Start with FAST_SMI_ONLY_ELEMENTS, and
transitions to
On 2011/10/18 15:42:36, Jakob wrote:
> nit: s/transitions/transition/

Done.

http://codereview.chromium.org/8258015/diff/9001/src/parser.cc#newcode3331
src/parser.cc:3331: for (int j = 0; j < i; ++j) {
On 2011/10/18 15:42:36, Jakob wrote:
> This code works, but it's brittle, as it subtly relies on the "if
(elements_kind
> == FAST_DOUBLE_ELEMENTS)" block being executed after the transition, which
might
> bite anyone who tries to refactor this. Please do either of these two:
> (1) Put in an explicit store of boilerplate_value->Number into the newly
> allocated double array (and, at your option, insert an "else" below to avoid
> duplication of the store), or
> (2) Add a comment explaining the fall-through situation.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode447
src/runtime.cc:447: // If the ElementKinds of the constant values of the array
literal are less
On 2011/10/18 15:42:36, Jakob wrote:
> nit: s/ElementKinds/ElementsKind/, s/are/is/

Done.

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode448
src/runtime.cc:448: // specific that the ElementsKind of the boilerplate array
object, change the
On 2011/10/18 15:42:36, Jakob wrote:
> nit: s/that/than/

Done.

http://codereview.chromium.org/8258015/diff/9001/src/runtime.cc#newcode452
src/runtime.cc:452: Handle<Map> smi_array_map =
isolate->factory()->GetElementsTransitionMap(
On 2011/10/18 15:42:36, Jakob wrote:
> nit: rename smi_array_map to something more generic (transitioned_array_map?
> new_array_map?), as it might well be a non-smi map.

Done.

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

http://codereview.chromium.org/8258015/diff/9001/src/x64/code-stubs-x64.cc#ne...
src/x64/code-stubs-x64.cc:291: // Copy the elements array.
On 2011/10/18 15:42:36, Jakob wrote:
> Duplicate line.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:1497: // Do tail-call to runtime routine.
On 2011/10/18 15:42:36, Jakob wrote:
> This is not a tail call.

Done.

http://codereview.chromium.org/8258015/diff/9001/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:1501: // Array literal has ElementsKind of
FAST_DOUBLE_ELEMENTS
On 2011/10/18 15:42:36, Jakob wrote:
> nit: missing dot at the end.

Done.

http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-t...
File test/mjsunit/array-literal-transitions.js (right):

http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-t...
test/mjsunit/array-literal-transitions.js:128: function d() {
On 2011/10/18 15:42:36, Jakob wrote:
> Isn't everything from here on down contained in test_large_literal() anyway?

Done.

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1534: __ CheckFastElements(r2, r3,
&double_elements);
Agreed. I will factor this stuff out into a shared stub that also tracks the
ElementsKind without reloading in another CL.
On 2011/10/19 08:10:45, Kevin Millikin wrote:
> This code checks the element kind for every stored computed value.  A small
> improvement to this code is to check up front, save the kind (tagged in the
> stack?) as a state, and update it when a transition is taken.

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1538: __ CheckFastSmiOnlyElements(r2, r3,
&fast_elements);
I modelled this after CheckMap, which also has a similar semantic. All of these
should probably be renamed to VerifyHasXXXX. Another CL, if you don't mind?
On 2011/10/19 08:10:45, Kevin Millikin wrote:
> It's a bit confusing (to me) that the label is for *not* fast smi only
elements.
>  That's because it seems more natural, given the function name, to interpret
it
> as something like "branch on fast smi only elements".

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1550: __ CallRuntime(Runtime::kSetProperty, 5);
Agreed. Next CL.
On 2011/10/19 08:10:45, Kevin Millikin wrote:
> Save this for a future CL, but I think that it would be better to have a
custom
> function for exactly setting an element as part of initializing an array
> literal.
> 
> The general SetProperty has a lot of stuff that we know we don't need
(checking
> attributes, strict vs. non-strict differences, proxies, etc.).

http://codereview.chromium.org/8258015/diff/7025/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:1556: __
StoreNumberToDoubleElements(result_register(),
I'll refactor this stuff into a separate stub in a follow-on CL.
On 2011/10/19 08:10:45, Kevin Millikin wrote:
> My biggest concern is that this is a lot of generated code (for the double
store
> particularly, but also the write barrier and all the rest) per-computed value.
> 
> It might be better to pass the value, array, index, and current element kind
to
> a stub (and get back a new element kind or update it in place), where the stub
> is a little interpreter for array stores implemented in assembly.
> 
> What do you think?