Further improvements upon r9747.

Further improvements upon r9747.

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

[Yang, 2011-10-24 09:07:49]

Follow-up on http://codereview.chromium.org/8359034/, including your
suggestions. Please take a look.

[Kevin Millikin (Chromium), 2011-10-24 09:31:29]

LGTM with comments below addressed.

http://codereview.chromium.org/8372028/diff/1/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/8372028/diff/1/src/arm/builtins-arm.cc#newcode150
src/arm/builtins-arm.cc:150: // Fill the FixedArray with the hole value.
Since initial_capacity can be zero, this should be:

if (initial_capacity > 0) {
  // Fill the FixedArray with the hole value.
  ASSERT_EQ(2 * kPointerSize, FixedArray::kHeaderSize);
  __ LoadRoot(scratch3, Heap::kTheHoleValueRootIndex);
  for (int i = 0; i < initial_capacity; i++) {
    __ str(scratch3, MemOperand(scratch1, kPointerSize, PostIndex));
  }
}

to avoid the useless load in the zero case.

You might also consider a static assert that the initial capacity is small
enough (say, not more than 10), with the comment that we should reconsider
unrolling the store loop in the case that the initial capacity is large.

http://codereview.chromium.org/8372028/diff/1/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

http://codereview.chromium.org/8372028/diff/1/src/x64/builtins-x64.cc#newcode...
src/x64/builtins-x64.cc:1033: __ Move(FieldOperand(result,
JSArray::kPropertiesOffset),
While you're at this code, you could eliminate FACTORY by

Factory* factory = masm->isolate()->factory();

http://codereview.chromium.org/8372028/diff/1/src/x64/builtins-x64.cc#newcode...
src/x64/builtins-x64.cc:1066: __ Move(scratch3, FACTORY->the_hole_value());
I think 

__ LoadRoot(scratch3, Heap::kTheHoleValueRootIndex);

is a smaller encoding of the load.

[Yang, 2011-10-24 10:02:33]

Implemented the alternative case to loop unrolling in arm. Please take another
look.

[Kevin Millikin (Chromium), 2011-10-24 11:11:50]

OK.

http://codereview.chromium.org/8372028/diff/3001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

http://codereview.chromium.org/8372028/diff/3001/src/arm/builtins-arm.cc#newc...
src/arm/builtins-arm.cc:155: if (false && initial_capacity <= kLoopUnfoldLimit)
{
I don't like the if (false ....  Dead code is not tested code, so it makes sense
to just get rid of it.

In this case, I think it's OK to have the unrolled loop only.