Inline array constructor.

Inline zero argument array constructor.
patch from issue 54583003 (dependent code).

Zero arguments - very easy

1 argument - three special cases:
  a) If length is a constant in valid array length range,
     no need to check it at runtime.
  b) respect DoNotInline feedback on the AllocationSite for
     cases that the argument is not a smi or is an integer
     with a length that should create a dictionary.
  c) if kind feedback is non-holey, and length is non-constant,
     we'd have to generate a lot of code to be correct.
     Don't inline this case.

N arguments - one special case:
  a) If a deopt ever occurs because an input argument isn't
     compatible with the elements kind, then set the
     DoNotInline flag.

BUG=
R=verwaest@chromium.org

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

[mvstanton, 2013-11-05 09:27:39]

PTAL. Inline array constructors. thx!
--Michael

[Toon Verwaest, 2013-11-06 16:42:44]

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#oldcode7284
src/hydrogen.cc:7284: HValue* constructor =
HPushArgument::cast(Top())->argument();
Why is it ok to merge this with the code above? The code above contains the
function on the TOS, this code contains an HPushArgument(function) on the TOS.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7187
src/hydrogen.cc:7187: HValue* argument = environment()->ExpressionStackAt(0);
Top();

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7192
src/hydrogen.cc:7192: int value = constant_argument->Integer32Value();
Rename value to array_size.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7203
src/hydrogen.cc:7203: : array_builder.AllocateArray(argument, argument, true);
Can we replace that "true" with an enum indicating the meaning?

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7270
src/hydrogen.cc:7270: ASSERT(argument_count <= 5);
Why 5? Can we at least make it a descriptive constant somewhere?

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7278
src/hydrogen.cc:7278: new_object = array_builder.AllocateArray(length, length,
fill_with_hole);
Why do we only fill with holes if the kind is smi? At least add a comment.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7306
src/hydrogen.cc:7306: if (argument_count <= 5) {
Why 5?

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7309
src/hydrogen.cc:7309: if (!site->DoNotInlineCall()) {
What about just CanInlineCall() rather than the double negative?

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7325
src/hydrogen.cc:7325: // inline this case.
Shouldn't we just generate a check whether the argument is 0 and bail out if
not?

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

https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:5767: Immediate(2));
I don't know what the line above means.

https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:5768: /*
leftover comment?

https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h#newcod...
src/objects-inl.h:1373: if (reason == TENURING) {
Can we make this a switch to statically ensure we catch all cases?

https://codereview.chromium.org/55933002/diff/170001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/55933002/diff/170001/src/objects.cc#newcode12752
src/objects.cc:12752: if (SitePointsToLiteral() &&
nit: merge lines

https://codereview.chromium.org/55933002/diff/170001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/55933002/diff/170001/src/objects.h#newcode8086
src/objects.h:8086: static const int kElementsKindMask = 0xffff;
Is there a reason why you don't use BitFields?

https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14667
src/runtime.cc:14667: Handle<AllocationSite> site;
Can we just declare it below, right above the if where it's initialized?

https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14691
src/runtime.cc:14691: if (!site.is_null() && !can_use_type_feedback) {
I find all these !site.is_null() a bit hard to read. Can we reorder the branches
and/or extract code into separate functions?

[mvstanton, 2013-11-07 16:34:04]

thx for comments Toon. I had to make a change to HForceRepresentation to restore
a lost optimization in BuildFillElementsWithHole(). And refactoring.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#oldcode7284
src/hydrogen.cc:7284: HValue* constructor =
HPushArgument::cast(Top())->argument();
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Why is it ok to merge this with the code above? The code above contains the
> function on the TOS, this code contains an HPushArgument(function) on the TOS.

Because I changed this whole else block to work like the block above: push
function and args onto the expression stack. Then, if we decide to make the
call, we use PreProcessCall() below to remove from the expression stack and turn
into PushArguments. The reason for that is in order to decide if the array call
is eligible for inlining I need to examine the expressions.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7187
src/hydrogen.cc:7187: HValue* argument = environment()->ExpressionStackAt(0);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Top();

Done.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7192
src/hydrogen.cc:7192: int value = constant_argument->Integer32Value();
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Rename value to array_size.

Done.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7203
src/hydrogen.cc:7203: : array_builder.AllocateArray(argument, argument, true);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Can we replace that "true" with an enum indicating the meaning?

Done. Doing this correctly took me on a refactoring journey into
code-stubs-hydrogen because the block just below (pushing capacity and length)
was essentially duplicated.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7270
src/hydrogen.cc:7270: ASSERT(argument_count <= 5);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Why 5? Can we at least make it a descriptive constant somewhere?

Done.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7278
src/hydrogen.cc:7278: new_object = array_builder.AllocateArray(length, length,
fill_with_hole);
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Why do we only fill with holes if the kind is smi? At least add a comment.

Done. Bailout could occur in the HStoreKeyed if we try to initialize a smi array
with a HeapNumber too big. So we have to leave the array without garbage if that
happens.

Also added a TODO that if all the arguments are constants in smi range then we
can eliminate the hole filling.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7306
src/hydrogen.cc:7306: if (argument_count <= 5) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Why 5?

Why indeed. I eliminated this as any predicate.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7309
src/hydrogen.cc:7309: if (!site->DoNotInlineCall()) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> What about just CanInlineCall() rather than the double negative?

Done.

https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7325
src/hydrogen.cc:7325: // inline this case.
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Shouldn't we just generate a check whether the argument is 0 and bail out if
> not?

I think it's a lot of trouble for a corner case (never happens in benchmarks).
We'd bail out and then have to set the AllocationSite::DoNotInline flag for the
next time around. But setting that flag is awkward. The bailout code would have
to recognize that it was called with completely valid arguments
(FAST_SMI_ELEMENTS, 0 < index < kInitialMaxFastElementArray), and that needs to
be the signal to set the DoNotInline flag. Alternatively, the DoNotInline flag
could be set just before the bailout. It just doesn't seem worth it.

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

https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:5767: Immediate(2));
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> I don't know what the line above means.

Trying to be clever adding 1 to a smi in memory. :p. I've cleaned this up with
some infrastructure that verifies you can add one to any packed elements kind to
get the holey version.

https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:5768: /*
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> leftover comment?

Done.

https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32...
src/ia32/code-stubs-ia32.cc:5915: __ and_(edx, Immediate(0xffff));
Fixed this up too.

https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h#newcod...
src/objects-inl.h:1373: if (reason == TENURING) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Can we make this a switch to statically ensure we catch all cases?

Yes, but I'm not happy that I have to return "something" at the end on some
compilers.

https://codereview.chromium.org/55933002/diff/170001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/55933002/diff/170001/src/objects.cc#newcode12752
src/objects.cc:12752: if (SitePointsToLiteral() &&
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> nit: merge lines

Done.

https://codereview.chromium.org/55933002/diff/170001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/55933002/diff/170001/src/objects.h#newcode8086
src/objects.h:8086: static const int kElementsKindMask = 0xffff;
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Is there a reason why you don't use BitFields?

Nope, now I do, thx!

https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14667
src/runtime.cc:14667: Handle<AllocationSite> site;
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> Can we just declare it below, right above the if where it's initialized?

Done.

https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14691
src/runtime.cc:14691: if (!site.is_null() && !can_use_type_feedback) {
On 2013/11/06 16:42:44, Toon Verwaest wrote:
> I find all these !site.is_null() a bit hard to read. Can we reorder the
branches
> and/or extract code into separate functions?

I pulled out turning the type_info into an allocationsite to the caller, and
consolidated the two points in the function where I set
site->SetDoNotInlineCall().

I think it looks better even though !site.is_null() still exists.

[Toon Verwaest, 2013-11-11 13:59:27]

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

https://codereview.chromium.org/55933002/diff/310001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:712: : JSArrayBuilder::DONT_FILL_WITH_HOLE;
Technically we don't need to fill this in with the hole. We could just fill it
in with 0 (memset).

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

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen-instruction...
src/hydrogen-instructions.h:1580: virtual int RedefinedOperandIndex() { return
0; }
Why is this needed? HForceRepresentation is explicitly eliminated after
inserting representation changes.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1568
src/hydrogen.cc:1568: HValue* new_object = (array_length == 0)
Remove the parentheses.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1578
src/hydrogen.cc:1578: HConstant* initial_capacity_node =
Add<HConstant>(initial_capacity);
Move this down to the case where initial-capacity-node is used.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1790
src/hydrogen.cc:1790: if (from->ActualValue()->IsConstant() &&
to->ActualValue()->IsConstant()) {
Please put this is a separate CL (that also more widely applies ActualValue to
values you expect to be HConstant).

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode2071
src/hydrogen.cc:2071: if (is_inlined_) {
Is it good enough to use IsStub() rather than is_inlined? Then we don't need an
enum after all.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode2124
src/hydrogen.cc:2124: return const_size;
I think constant folding should take care of this. (And in other places where
you added such code, if there are such).

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7234
src/hydrogen.cc:7234: ASSERT(!environment()->ExpressionStackIsEmpty());
I this ASSERT and the comment necessary? Doesn't the expression below fail if
ExpressionStackAt(...) is out of bounds? If it doesn't, can we just make that
fail?

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7246
src/hydrogen.cc:7246: // to avoid creating a packed non-empty array (not
kosher).
I'd remove the (not kosher) part.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7247
src/hydrogen.cc:7247: if (argument_count == 1 && !IsHoleyElementsKind(kind)) {
Can't we just do this in BuildAllocateArrayFromLength? That would seem better
encapsulated.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7336
src/hydrogen.cc:7336: inline_ok = IsHoleyElementsKind(expr->elements_kind());
Shouldn't we just deopt if the passed length is non-zero?

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h#newcode1544
src/hydrogen.h:1544: bool is_inlined = false);
Please make this an enum.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h#newcode1586
src/hydrogen.h:1586: bool is_inlined_;
enum

https://codereview.chromium.org/55933002/diff/310001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/55933002/diff/310001/src/objects-inl.h#newcod...
src/objects-inl.h:1380: default:
No need to add a default case if all cases are handled. Actually better to not
add a default case, so the compiler will complain when a new, unsupported case
is added to that enum.

https://codereview.chromium.org/55933002/diff/310001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/55933002/diff/310001/src/objects.h#newcode8109
src/objects.h:8109: value |= (1 << 16);
Remove the line above.

[mvstanton, 2013-11-13 14:12:51]

Hi Toon, addressed feedback.
I extracted the HForceRepresentation idef into another CL I should land first.
Thx,
--Michael

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

https://codereview.chromium.org/55933002/diff/310001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:712: : JSArrayBuilder::DONT_FILL_WITH_HOLE;
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Technically we don't need to fill this in with the hole. We could just fill it
> in with 0 (memset).

I've added a TODO for this, it's a neat idea.

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

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen-instruction...
src/hydrogen-instructions.h:1580: virtual int RedefinedOperandIndex() { return
0; }
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Why is this needed? HForceRepresentation is explicitly eliminated after
> inserting representation changes.

It's done so we can make optimizations based on seeing HConstant in
JSArrayBuilder. Per our discussion, I'll pull this into a separate CL.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1568
src/hydrogen.cc:1568: HValue* new_object = (array_length == 0)
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Remove the parentheses.

Done.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1578
src/hydrogen.cc:1578: HConstant* initial_capacity_node =
Add<HConstant>(initial_capacity);
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Move this down to the case where initial-capacity-node is used.

Done.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode1790
src/hydrogen.cc:1790: if (from->ActualValue()->IsConstant() &&
to->ActualValue()->IsConstant()) {
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Please put this is a separate CL (that also more widely applies ActualValue to
> values you expect to be HConstant).

I made another CL that isolates the idef of HForceRepresentation that was
needed. But the wider application is so wide that I'd like to think about the
best solution a bit. My small CL will solve it locally here and then a follow up
will happen.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode2071
src/hydrogen.cc:2071: if (is_inlined_) {
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Is it good enough to use IsStub() rather than is_inlined? Then we don't need
an
> enum after all.

Sure is, thanks for the idea. Done.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode2124
src/hydrogen.cc:2124: return const_size;
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> I think constant folding should take care of this. (And in other places where
> you added such code, if there are such).

Indeed, thx!

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7234
src/hydrogen.cc:7234: ASSERT(!environment()->ExpressionStackIsEmpty());
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> I this ASSERT and the comment necessary? Doesn't the expression below fail if
> ExpressionStackAt(...) is out of bounds? If it doesn't, can we just make that
> fail?

Done.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7246
src/hydrogen.cc:7246: // to avoid creating a packed non-empty array (not
kosher).
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> I'd remove the (not kosher) part.

Done.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7247
src/hydrogen.cc:7247: if (argument_count == 1 && !IsHoleyElementsKind(kind)) {
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Can't we just do this in BuildAllocateArrayFromLength? That would seem better
> encapsulated.

Hmm, I'd rather not because then the JSArrayBuilder would have to allow you to
change ElementsKind, currently passed in at constructor time. This is a special
case, and I think it's at the right place for now. I like the semantics that
kind becomes fixed once you have a JSArrayBuilder.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.cc#newcode7336
src/hydrogen.cc:7336: inline_ok = IsHoleyElementsKind(expr->elements_kind());
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Shouldn't we just deopt if the passed length is non-zero?

Yep, done. I also added a test in array-constructor-feedback.js to ensure this
works as expected.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h#newcode1544
src/hydrogen.h:1544: bool is_inlined = false);
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Please make this an enum.

Removed per your other feedback.

https://codereview.chromium.org/55933002/diff/310001/src/hydrogen.h#newcode1586
src/hydrogen.h:1586: bool is_inlined_;
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> enum

Removed the member.

https://codereview.chromium.org/55933002/diff/310001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/55933002/diff/310001/src/objects-inl.h#newcod...
src/objects-inl.h:1380: default:
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> No need to add a default case if all cases are handled. Actually better to not
> add a default case, so the compiler will complain when a new, unsupported case
> is added to that enum.

Done.

https://codereview.chromium.org/55933002/diff/310001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/55933002/diff/310001/src/objects.h#newcode8109
src/objects.h:8109: value |= (1 << 16);
On 2013/11/11 13:59:28, Toon Verwaest wrote:
> Remove the line above.

oops!

[Toon Verwaest, 2013-11-13 14:59:39]

lgtm

[mvstanton, 2013-11-13 20:42:23]

Hi Toon, will you quickly check over the ARM & x64 ports in
code-stubs-<platform>.cc?
All the best,
--Michael

[mvstanton, 2013-11-14 12:05:27]

Committed patchset #13 manually as r17741 (presubmit successful).