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#newcode1549 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#newcode1553 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#newcode1563 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.cc#newcode1528 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.cc#newcode1532 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#newcode291 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#newcode1497 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#newcode1501 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-transitions.js File test/mjsunit/array-literal-transitions.js (right): http://codereview.chromium.org/8258015/diff/9001/test/mjsunit/array-literal-transitions.js#newcode128 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#newcode1534 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#newcode1538 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#newcode1550 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#newcode1556 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?
http://codereview.chromium.org/8258015/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
