Reland r20974: Unify and simplify the FastCloneShallowArrayStub

Reland r20974: Unify and simplify the FastCloneShallowArrayStub

- Don't bake in length/capacity into full codegen calls of stubs,
allowing boilerplates to increase their capacity without regenerating
code.
- Unify all variants of the clone stub into a single,
length-independent version.
- Various tweaks to make sure that the clone stub doesn't spill and
therefore need an eager stack frame.
- Handle all lengths of array literals in the fast case.

R=mvstanton@chromium.org

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

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

[danno, 2014-05-07 14:14:53]

PTAL, this version doesn't try to tweak the register allocation like the last
one (which was wrong).

[mvstanton, 2014-05-09 13:16:48]

lgtm, modulo nits and request-for-comments.

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

https://codereview.chromium.org/272513004/diff/110001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:649: result =
BuildArraySingleArgumentConstructor(&array_builder);
It would be good if the single arg case avoided a frame too, does it run into
problems to add it there?

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

https://codereview.chromium.org/272513004/diff/110001/src/code-stubs.h#newcod...
src/code-stubs.h:281: Representation* register_param_representations_;
Could you add a comment that the default Representation is Tagged() if the
register_param_representations_ pointer is left in default (NULL) state?

https://codereview.chromium.org/272513004/diff/110001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/272513004/diff/110001/src/counters.h#newcode382
src/counters.h:382: SC(inlined_copyied_elements, V8.InlinedCopiedElements)      
       \
s/copyied/copied/

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen-instructio...
src/hydrogen-instructions.h:621: // Indicates an instruction shouldn't be
replaced by optimization.
Or alternatively, instead of commenting the usage of kCantBeReplaced in
hydrogen.cc, expand on this comment a bit to indicate why you might want to use
this (reload is cheaper than spilling, say).

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2474: if (to == NULL) to = AddLoadFixedArrayLength(elements);
nit: I'd rather see a { } around the assignment, the one line style is fine for
function call or "return;" or things like that.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2562: if (capacity == NULL) capacity =
AddLoadFixedArrayLength(to_elements);
nit: same here.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2712: boilerplate_elements->SetFlag(HValue::kCantBeReplaced);
Can you comment the motivation for using kCantBeReplaced here? I think this is
to avoid a load from spill slot, right?

https://codereview.chromium.org/272513004/diff/110001/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/272513004/diff/110001/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:1732: if (expr->depth() > 1) {
Awesome!

https://codereview.chromium.org/272513004/diff/110001/src/lithium.cc
File src/lithium.cc (right):

https://codereview.chromium.org/272513004/diff/110001/src/lithium.cc#newcode453
src/lithium.cc:453: generator.NeedsEagerFrame()));
+1.

[danno, 2014-05-09 15:51:24]

feedback addressed, landing...

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

https://codereview.chromium.org/272513004/diff/110001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:649: result =
BuildArraySingleArgumentConstructor(&array_builder);
On 2014/05/09 13:16:49, mvstanton wrote:
> It would be good if the single arg case avoided a frame too, does it run into
> problems to add it there?

Yes. Currently asserts, but I will fix this in a subsequent CL.

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

https://codereview.chromium.org/272513004/diff/110001/src/code-stubs.h#newcod...
src/code-stubs.h:281: Representation* register_param_representations_;
On 2014/05/09 13:16:49, mvstanton wrote:
> Could you add a comment that the default Representation is Tagged() if the
> register_param_representations_ pointer is left in default (NULL) state?

Done.

https://codereview.chromium.org/272513004/diff/110001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/272513004/diff/110001/src/counters.h#newcode382
src/counters.h:382: SC(inlined_copyied_elements, V8.InlinedCopiedElements)      
       \
On 2014/05/09 13:16:49, mvstanton wrote:
> s/copyied/copied/

Done.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen-instructio...
src/hydrogen-instructions.h:621: // Indicates an instruction shouldn't be
replaced by optimization.
On 2014/05/09 13:16:49, mvstanton wrote:
> Or alternatively, instead of commenting the usage of kCantBeReplaced in
> hydrogen.cc, expand on this comment a bit to indicate why you might want to
use
> this (reload is cheaper than spilling, say).

Done.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2474: if (to == NULL) to = AddLoadFixedArrayLength(elements);
On 2014/05/09 13:16:49, mvstanton wrote:
> nit: I'd rather see a { } around the assignment, the one line style is fine
for
> function call or "return;" or things like that.

Done.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2474: if (to == NULL) to = AddLoadFixedArrayLength(elements);
On 2014/05/09 13:16:49, mvstanton wrote:
> nit: I'd rather see a { } around the assignment, the one line style is fine
for
> function call or "return;" or things like that.

Done.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2562: if (capacity == NULL) capacity =
AddLoadFixedArrayLength(to_elements);
On 2014/05/09 13:16:49, mvstanton wrote:
> nit: same here.

Done.

https://codereview.chromium.org/272513004/diff/110001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2712: boilerplate_elements->SetFlag(HValue::kCantBeReplaced);
On 2014/05/09 13:16:49, mvstanton wrote:
> Can you comment the motivation for using kCantBeReplaced here? I think this is
> to avoid a load from spill slot, right?

Done.

[danno, 2014-05-09 15:56:07]

Committed patchset #10 manually as r21230 (presubmit successful).

[danno, 2014-05-12 07:41:56]

PTAL

[mvstanton, 2014-05-12 07:45:07]

lgtm

[danno, 2014-05-12 07:49:28]

Committed patchset #11 manually as r21253 (presubmit successful).

[Igor Sheludko, 2014-05-28 13:21:03]

https://codereview.chromium.org/272513004/diff/190001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/272513004/diff/190001/src/hydrogen.cc#newcode...
src/hydrogen.cc:2532: // Unroll the loop for small elements kinds.
DBC: I'm afraid that this case still requires pre-filling with holes for certain
combinations of elements kinds.