Hydrogen stubs for array constructors

Constructed arrays can be created with Hydrogen code stubs. The feature is still off by default (--optimize-constructed-arrays).

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14441

[mvstanton, 2013-03-07 10:21:46]

This isn't ready for review, but it could be useful for discussion points.

[mvstanton, 2013-03-22 16:44:59]

Hi Danno, I'm going on vacation next week, and as discussed there are pieces of
this CL that can hive off into separate checkins. While I'm away, if you can
look at your leisure at three things:

1) code-stubs-hydrogen.cc
2) hydrogen.cc
3) code-stubs-ia32.cc

The first two make up the "nut" of the work, with 3) describing the dispatch
table. Most of the other changes will end up in separate CLs.
Thanks for the look,
--Michael

[mvstanton, 2013-04-10 09:03:43]

Hi guys, I'll ask Hannes to have a good look at this one, with Danno for
overview.
Thanks,
--Michael

ps - platforms are not complete, and I'll wait for comments on ia32.

[mvstanton, 2013-04-15 07:45:36]

Adding Toon.

[mvstanton, 2013-04-15 15:14:49]

Some comments after review with Hannes.

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1690
src/hydrogen.cc:1690: Isolate* isolate = builder()->graph()->isolate();
Hannes: How about for symmetry with BuildCopyObjectHeader, have a
BuildObjectHeader that deals with the header portion of the array we are
creating.

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1699
src/hydrogen.cc:1699: HAllocate* new_object = new(zone()) HAllocate(context,
size_in_bytes,
Hannes: How about if HAllocate deals with the ALLOCATE_DOUBLE_ALIGNED_FLAG?)

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1732
src/hydrogen.cc:1732: if (mode_ == TRACK_ALLOCATION_SITE) {
Hannes: Factor this out to combine with the piece of code in BuildEmitDeepCopy
that builds the AllocationSiteInfo object.

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1805
src/hydrogen.cc:1805: IsFastObjectElementsKind(kind_))
lines above replace with IsFastDoubleElementsKind(kind_).

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1813
src/hydrogen.cc:1813: if (nominally_empty && initial_capacity() <=
kLoopUnfoldLimit) {
move kLoopUnfoldLimit up to initial_capacity() definition point (and coalesce
with builtins-ia32.cc version of kLoopUnfoldLimit.

https://codereview.chromium.org/12385014/diff/55001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/12385014/diff/55001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:7690: for (int i = 0; i <= last_index; ++i) {
Hannes: why not have a real table here, instead of a bunch of conditionals.

[mvstanton, 2013-04-16 11:48:52]

Hi Hannes, here is the review response. Let me know how it goes. Best,
--Michael

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1690
src/hydrogen.cc:1690: Isolate* isolate = builder()->graph()->isolate();
On 2013/04/15 15:14:49, mvstanton wrote:
> Hannes: How about for symmetry with BuildCopyObjectHeader, have a
> BuildObjectHeader that deals with the header portion of the array we are
> creating.

Done. And I continued in this spirit, finding several more refactorings that
allowed sharing with other recently created "Builder" code.

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1699
src/hydrogen.cc:1699: HAllocate* new_object = new(zone()) HAllocate(context,
size_in_bytes,
On 2013/04/15 15:14:49, mvstanton wrote:
> Hannes: How about if HAllocate deals with the ALLOCATE_DOUBLE_ALIGNED_FLAG?)

I made a useful helper function to improve this. It still seemed difficult and
outside of scope to completely hide flags inside the HAllocate class.

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1732
src/hydrogen.cc:1732: if (mode_ == TRACK_ALLOCATION_SITE) {
On 2013/04/15 15:14:49, mvstanton wrote:
> Hannes: Factor this out to combine with the piece of code in BuildEmitDeepCopy
> that builds the AllocationSiteInfo object.

Done.

https://codereview.chromium.org/12385014/diff/55001/src/hydrogen.cc#newcode1805
src/hydrogen.cc:1805: IsFastObjectElementsKind(kind_))
On 2013/04/15 15:14:49, mvstanton wrote:
> lines above replace with IsFastDoubleElementsKind(kind_). 

Done. Actually I could merge this function with another and remove it
completely.

https://codereview.chromium.org/12385014/diff/55001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/12385014/diff/55001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:7690: for (int i = 0; i <= last_index; ++i) {
On 2013/04/15 15:14:49, mvstanton wrote:
> Hannes: why not have a real table here, instead of a bunch of conditionals.

I talked about this with Jakob and Yang, who pointed out that it's awkward to
make a real "data table" approach because our assembler doesn't support it
directly. Then Toon pointed out that the table would cause an expensive pipeline
stall anyway...at least the way it is now the branch predictor might make a good
choice (statistically, the first branch is probably enough, for smi arrays).
Finally, this code isn't called once we are in the crankshafted version of new
Array(..)

[Hannes Payer (out of office), 2013-04-18 11:14:39]

Looks pretty good already, another round of comments. I am waiting for your
update which splits up this CL.

https://codereview.chromium.org/12385014/diff/63028/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:105: // r2 -- type info cell with elements kind
why is r1 not used?

https://codereview.chromium.org/12385014/diff/63028/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/builtins.cc#newcode203
src/builtins.cc:203: // ASSERT(args.length() == 2);
Is this ASSERT still needed?

https://codereview.chromium.org/12385014/diff/63028/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/12385014/diff/63028/src/builtins.h#newcode203
src/builtins.h:203: Code::kNoExtraICState)              \
can you bring back the empty line?

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:453: // AddInstruction(new(zone())
HCheckSmi(argument));
I think you can remove that line.

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.cc#newcode654
src/code-stubs.cc:654: if (descriptor->register_param_count_ < 0) {
could you add a comment explaining what is going on there?

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1433
src/code-stubs.h:1433: explicit ArrayConstructorStubBase(ElementsKind kind,
explicit is only used in the one argument case

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1470
src/code-stubs.h:1470: AllocationSiteMode mode = TRACK_ALLOCATION_SITE) :
explicit is only used in the one argument case

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1490
src/code-stubs.h:1490: AllocationSiteMode mode = TRACK_ALLOCATION_SITE) :
explicit is only used in the one argument case

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1510
src/code-stubs.h:1510: AllocationSiteMode mode = TRACK_ALLOCATION_SITE) :
explicit is only used in the one argument case

https://codereview.chromium.org/12385014/diff/63028/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/compiler.cc#newcode129
src/compiler.cc:129: return -1;
Do we need that special case?

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen-instructions...
src/hydrogen-instructions.h:4890: }
add newline

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1338
src/hydrogen.cc:1338: JSArray::kLengthOffset));
I think you have to take care of the GVN flags here.
SetGVNFlag(kChangesArrayLengths);

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1361
src/hydrogen.cc:1361: JSArray::kElementsOffset));
I think you have to take care of the GVN flags here.
SetGVNFlag(kChangesElementsPointer);

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1680
src/hydrogen.cc:1680: // Determine mode
you can remove the comment, the following code should be self explaining

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1692
src/hydrogen.cc:1692: HGlobalObject(context));
Why do you need the global_object?

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode9291
src/hydrogen.cc:9291: // TODO(mvstanton): This is a bug because it makes a new
JS property cell.
As discussed offline, this is not a bug but one could do better. Maybe update
the comment to reflect that.

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode9293
src/hydrogen.cc:9293: // code gen case. The right way to do that is to begin
passing the cell to
Instead

https://codereview.chromium.org/12385014/diff/63028/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:2537: // int3();
is this debugging leftover code?

https://codereview.chromium.org/12385014/diff/63028/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/objects.cc#newcode8400
src/objects.cc:8400: //        native_context->js_array_maps()->IsUndefined()) {
are you planning to remove this?

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/allocation-s...
File test/mjsunit/allocation-site-info.js (right):

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:156: for(i=0;i<2;i++) {
codestyle: for (i=0; i<2; i++) {

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/compiler/inl...
File test/mjsunit/compiler/inline-accessors.js (right):

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/compiler/inl...
test/mjsunit/compiler/inline-accessors.js:32: // TODO(mvstanton): use this code
as a template to be able to test deoptimization.
linewidth 80

[mvstanton, 2013-04-18 13:39:26]

Hi Hannes, I pulled many things out and responded to comments.

However, I couldn't easily remove a lithium piece, which hangs together with
something in code-stubs-hydrogen: the HReturn instruction emission. Also an ARM
bug fix in the HReturn code generation.

https://codereview.chromium.org/12385014/diff/63028/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:105: // r2 -- type info cell with elements kind
On 2013/04/18 11:14:39, Hannes Payer wrote:
> why is r1 not used?

Good question and there is a decent answer. Each such parameter to a hydrogen
function burns a register, so we should only do it if we actually need it. The
only thing we'd use r1 for is to verify that the function called is really the
array function (deopting if not). As it turns out, we do that outside the call
(in hydrogen.cc). So, we only use one parameter, yay.

https://codereview.chromium.org/12385014/diff/63028/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/builtins.cc#newcode203
src/builtins.cc:203: // ASSERT(args.length() == 2);
On 2013/04/18 11:14:39, Hannes Payer wrote:
> Is this ASSERT still needed?

It's good to have, and I should enable the darn thing. Enabled.

https://codereview.chromium.org/12385014/diff/63028/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/12385014/diff/63028/src/builtins.h#newcode203
src/builtins.h:203: Code::kNoExtraICState)              \
On 2013/04/18 11:14:39, Hannes Payer wrote:
> can you bring back the empty line?

Done.

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:453: // AddInstruction(new(zone())
HCheckSmi(argument));
On 2013/04/18 11:14:39, Hannes Payer wrote:
> I think you can remove that line.
thx!, done

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.cc#newcode654
src/code-stubs.cc:654: if (descriptor->register_param_count_ < 0) {
On 2013/04/18 11:14:39, Hannes Payer wrote:
> could you add a comment explaining what is going on there?

It's checking if the descriptor has been initialized or not. I added a "bool
initialized() const;" method to the struct to make it self-explanatory.

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1433
src/code-stubs.h:1433: explicit ArrayConstructorStubBase(ElementsKind kind,
On 2013/04/18 11:14:39, Hannes Payer wrote:
> explicit is only used in the one argument case

Done.

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1470
src/code-stubs.h:1470: AllocationSiteMode mode = TRACK_ALLOCATION_SITE) :
On 2013/04/18 11:14:39, Hannes Payer wrote:
> explicit is only used in the one argument case

Done.

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1490
src/code-stubs.h:1490: AllocationSiteMode mode = TRACK_ALLOCATION_SITE) :
On 2013/04/18 11:14:39, Hannes Payer wrote:
> explicit is only used in the one argument case

Done.

https://codereview.chromium.org/12385014/diff/63028/src/code-stubs.h#newcode1510
src/code-stubs.h:1510: AllocationSiteMode mode = TRACK_ALLOCATION_SITE) :
On 2013/04/18 11:14:39, Hannes Payer wrote:
> explicit is only used in the one argument case

Done.

https://codereview.chromium.org/12385014/diff/63028/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/compiler.cc#newcode129
src/compiler.cc:129: return -1;
On 2013/04/18 11:14:39, Hannes Payer wrote:
> Do we need that special case?

Indeed no, and I protected the region with ASSERT(!IsStub()), because stubs
should use another method for determining num parameters.

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen-instructions...
src/hydrogen-instructions.h:4890: }
On 2013/04/18 11:14:39, Hannes Payer wrote:
> add newline

Done.

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1338
src/hydrogen.cc:1338: JSArray::kLengthOffset));
On 2013/04/18 11:14:39, Hannes Payer wrote:
> I think you have to take care of the GVN flags here.
> SetGVNFlag(kChangesArrayLengths);

Done.

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1361
src/hydrogen.cc:1361: JSArray::kElementsOffset));
On 2013/04/18 11:14:39, Hannes Payer wrote:
> I think you have to take care of the GVN flags here.
> SetGVNFlag(kChangesElementsPointer);

Done.

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1680
src/hydrogen.cc:1680: // Determine mode
On 2013/04/18 11:14:39, Hannes Payer wrote:
> you can remove the comment, the following code should be self explaining

Done.

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1692
src/hydrogen.cc:1692: HGlobalObject(context));
On 2013/04/18 11:14:39, Hannes Payer wrote:
> Why do you need the global_object?

You mean instead of saying isolate()->native_context()?

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode9291
src/hydrogen.cc:9291: // TODO(mvstanton): This is a bug because it makes a new
JS property cell.
On 2013/04/18 11:14:39, Hannes Payer wrote:
> As discussed offline, this is not a bug but one could do better. Maybe update
> the comment to reflect that.

Done.

https://codereview.chromium.org/12385014/diff/63028/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:2537: // int3();
On 2013/04/18 11:14:39, Hannes Payer wrote:
> is this debugging leftover code?

Yes and no, it is actually a bug because the j(equal..) instruction is
unnecessary on the left. It emits the same code as the Check(...) line just
below.

This is unrelated, I will pull it out of the CL.

https://codereview.chromium.org/12385014/diff/63028/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/objects.cc#newcode8400
src/objects.cc:8400: //        native_context->js_array_maps()->IsUndefined()) {
On 2013/04/18 11:14:39, Hannes Payer wrote:
> are you planning to remove this?

Done.

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/allocation-s...
File test/mjsunit/allocation-site-info.js (right):

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/allocation-s...
test/mjsunit/allocation-site-info.js:156: for(i=0;i<2;i++) {
On 2013/04/18 11:14:39, Hannes Payer wrote:
> codestyle: for (i=0; i<2; i++) {

Done.

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/compiler/inl...
File test/mjsunit/compiler/inline-accessors.js (right):

https://codereview.chromium.org/12385014/diff/63028/test/mjsunit/compiler/inl...
test/mjsunit/compiler/inline-accessors.js:32: // TODO(mvstanton): use this code
as a template to be able to test deoptimization.
On 2013/04/18 11:14:39, Hannes Payer wrote:
> linewidth 80

Thx, I was able to remove the TODO. As it turns out, existing tests do all the
deopts required, by doing things like passing a non-Smi to single arg array
constructor.

[mvstanton, 2013-04-18 14:43:01]

Comments from review with Danno.

https://codereview.chromium.org/12385014/diff/91001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/12385014/diff/91001/src/bootstrapper.cc#newco...
src/bootstrapper.cc:904:
isolate->builtins()->builtin(Builtins::kInternalArrayConstructCode));
wrong builtin.

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:137: stack_parameter_count,
try not to use HChange explicitly, can you avoid?

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:161: if (stack_parameter_count->IsParameter() &&
change to !stack_parameter_count->IsConstant()

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:492: JSArrayBuilder array_builder(this,
nit: this should be on next line.

https://codereview.chromium.org/12385014/diff/91001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/12385014/diff/91001/src/flag-definitions.h#ne...
src/flag-definitions.h:249: DEFINE_bool(optimize_constructed_arrays, true,
turn off by default!

[Hannes Payer (out of office), 2013-04-23 11:42:50]

lgtm with a few nits, coordinate with danno after addressing his comments

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/63028/src/hydrogen.cc#newcode1692
src/hydrogen.cc:1692: HGlobalObject(context));
On 2013/04/18 13:39:26, mvstanton wrote:
> On 2013/04/18 11:14:39, Hannes Payer wrote:
> > Why do you need the global_object?
> 
> You mean instead of saying isolate()->native_context()?
> 

Never mind, keep it...

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:102: int constant_stack_parameter_count) {
can you describe the semantic of constant_stack_parameter_count in a comment,
e.g. the -1 case. applies also for the other architectures.

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:104: // r2 -- type info cell with elements kind
do you want to bring back the comment for r0? since it is used below...

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:7185:
stub1.GetCode(isolate)->set_is_pregenerated(true);
Why did you remove that comment?

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.h...
src/arm/code-stubs-arm.h:38: Label* call_generic_code);
does that fit in one line?

https://codereview.chromium.org/12385014/diff/102001/src/arm/lithium-codegen-...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/12385014/diff/102001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:2950: __ SmiUntag(reg);  // it is a smi
codestyle, move comment in a separate line before code

https://codereview.chromium.org/12385014/diff/102001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12385014/diff/102001/src/code-stubs.h#newcode639
src/code-stubs.h:639: explicit ArrayConstructorStub(Isolate* isolate, int
argument_count);
explicit is just used in the single argument case

[mvstanton, 2013-04-23 14:19:57]

Thanks guys,
Here is the update. The most serious change is in hydrogen-instructions.h,
HChange::HChange(), responding to Danno's comment to remove the explicit HChange
call in hydrogen code stub generation.

I've actually got that change in a separate CL too, and should land it soon.

https://codereview.chromium.org/12385014/diff/91001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/12385014/diff/91001/src/bootstrapper.cc#newco...
src/bootstrapper.cc:904:
isolate->builtins()->builtin(Builtins::kInternalArrayConstructCode));
On 2013/04/18 14:43:01, mvstanton wrote:
> wrong builtin.

Actually it's not wrong, it's just confusingly named. Renamed it.

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:137: stack_parameter_count,
On 2013/04/18 14:43:01, mvstanton wrote:
> try not to use HChange explicitly, can you avoid?

I was able to avoid it by setting the type of stack_parameter_count to Smi. This
makes sense because I know the range for # of arguments is 0
Code::kMaxArgumentCount, which is within smi.

I had to make another change in the HChange constructor though in order to
preserve write barrier free field stores for everything downwind of the HChange.

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:161: if (stack_parameter_count->IsParameter() &&
On 2013/04/18 14:43:01, mvstanton wrote:
> change to !stack_parameter_count->IsConstant()

Done.

https://codereview.chromium.org/12385014/diff/91001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:492: JSArrayBuilder array_builder(this,
On 2013/04/18 14:43:01, mvstanton wrote:
> nit: this should be on next line.

done, also in another place in this file.

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:104: // r2 -- type info cell with elements kind
On 2013/04/23 11:42:50, Hannes Payer wrote:
> do you want to bring back the comment for r0? since it is used below...

good point, thx, done.

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:7185:
stub1.GetCode(isolate)->set_is_pregenerated(true);
On 2013/04/23 11:42:50, Hannes Payer wrote:
> Why did you remove that comment?

I guess text angers me. I have put it back! :)

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.h
File src/arm/code-stubs-arm.h (right):

https://codereview.chromium.org/12385014/diff/102001/src/arm/code-stubs-arm.h...
src/arm/code-stubs-arm.h:38: Label* call_generic_code);
On 2013/04/23 11:42:50, Hannes Payer wrote:
> does that fit in one line?

Done.

https://codereview.chromium.org/12385014/diff/102001/src/arm/lithium-codegen-...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/12385014/diff/102001/src/arm/lithium-codegen-...
src/arm/lithium-codegen-arm.cc:2950: __ SmiUntag(reg);  // it is a smi
On 2013/04/23 11:42:50, Hannes Payer wrote:
> codestyle, move comment in a separate line before code

done, on all platforms

https://codereview.chromium.org/12385014/diff/102001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12385014/diff/102001/src/code-stubs.h#newcode639
src/code-stubs.h:639: explicit ArrayConstructorStub(Isolate* isolate, int
argument_count);
On 2013/04/23 11:42:50, Hannes Payer wrote:
> explicit is just used in the single argument case

Done.

[danno, 2013-04-25 14:49:27]

lgtm

lgtm

[mvstanton, 2013-04-25 16:00:55]

Committed patchset #15 manually as r14441 (presubmit successful).