Inline array constructor.

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 2e05654ef82046552d6abb929ad7ede0f2f79089..a7dc07e3777eccfbae671817a538e74cb2c80137 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -2067,6 +2067,16 @@ HValue* HGraphBuilder::JSArrayBuilder::EstablishAllocationSize(
   STATIC_ASSERT(FixedDoubleArray::kHeaderSize == FixedArray::kHeaderSize);
   base_size += FixedArray::kHeaderSize;
 
+  if (length_node->IsConstant()) {
+    HConstant* constant_length_node = HConstant::cast(length_node);
+    if (constant_length_node->HasInteger32Value()) {
+      int size = base_size + elements_size() *
+          constant_length_node->Integer32Value();
+      HInstruction* const_size = builder()->Add<HConstant>(size);
+      return const_size;
+    }
+  }
+
   HInstruction* elements_size_value =
       builder()->Add<HConstant>(elements_size());
   HInstruction* mul = builder()->Add<HMul>(length_node, elements_size_value);
@@ -7169,6 +7179,116 @@ void HOptimizedGraphBuilder::VisitCall(Call* expr) {
 }
 
 
+HValue* HOptimizedGraphBuilder::BuildInlinedCallNewArray_OneArg(
+    CallNew* expr,
+    HValue* constructor,
+    HInstruction* cell_instruction) {
+  ElementsKind kind = expr->elements_kind();
+  HValue* argument = environment()->ExpressionStackAt(0);

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

Top();

[mvstanton, 2013/11/07 16:34:05]

Done.

+  HValue* new_object;
+  if (argument->IsConstant()) {
+    HConstant* constant_argument = HConstant::cast(argument);
+    ASSERT(constant_argument->HasSmiValue());
+    int value = constant_argument->Integer32Value();

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

Rename value to array_size.

[mvstanton, 2013/11/07 16:34:05]

Done.

+    JSArrayBuilder array_builder(this,
+                                 value == 0
+                                   ? kind
+                                   : GetHoleyElementsKind(kind),
+                                 cell_instruction,
+                                 constructor,
+                                 DISABLE_ALLOCATION_SITES);
+
+    new_object = (value == 0)
+        ? array_builder.AllocateEmptyArray()
+        : array_builder.AllocateArray(argument, argument, true);

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

Can we replace that "true" with an enum indicating the meaning?

[mvstanton, 2013/11/07 16:34:05]

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.

+  } else {
+    ASSERT(IsHoleyElementsKind(kind));
+
+    // Smi check and range check on the input arg.
+    HValue* constant_zero = graph()->GetConstant0();
+    HConstant* max_alloc_length =
+        Add<HConstant>(JSObject::kInitialMaxFastElementArray);
+    const int initial_capacity = JSArray::kPreallocatedArrayElements;
+    HConstant* initial_capacity_node = Add<HConstant>(initial_capacity);
+    HInstruction* checked_arg = Add<HBoundsCheck>(argument, max_alloc_length);
+    IfBuilder if_builder(this);
+    if_builder.If<HCompareNumericAndBranch>(checked_arg, constant_zero,
+                                            Token::EQ);
+
+    if_builder.Then();
+    Push(initial_capacity_node);    // capacity
+    Push(constant_zero);            // length
+    if_builder.Else();
+    Push(checked_arg);              // capacity
+    Push(checked_arg);              // length
+    if_builder.End();
+
+    // Figure out total size
+    HValue* length = Pop();
+    HValue* capacity = Pop();
+    JSArrayBuilder array_builder(this,
+                                 kind,
+                                 cell_instruction,
+                                 constructor,
+                                 DISABLE_ALLOCATION_SITES);
+    new_object = array_builder.AllocateArray(capacity, length, true);
+  }
+
+  return new_object;
+}
+
+
+void HOptimizedGraphBuilder::BuildInlinedCallNewArray(CallNew* expr) {
+  NoObservableSideEffectsScope no_effects(this);
+
+  int argument_count = expr->arguments()->length();
+  // We should at least have the constructor on the expression stack.
+  ASSERT(!environment()->ExpressionStackIsEmpty());
+  HValue* constructor = environment()->ExpressionStackAt(argument_count);
+
+  ElementsKind kind = expr->elements_kind();
+  Handle<Cell> cell = expr->allocation_info_cell();
+  AllocationSite* site = AllocationSite::cast(cell->value());
+
+  // Register on the site for deoptimization if the cell value changes.
+  site->AddDependentCompilationInfo(AllocationSite::TRANSITIONS, top_info());
+  HInstruction* cell_instruction = Add<HConstant>(cell);
+
+  // Build the array.
+  HValue* new_object;
+  if (argument_count == 0) {
+    JSArrayBuilder array_builder(this,
+                                 kind,
+                                 cell_instruction,
+                                 constructor,
+                                 DISABLE_ALLOCATION_SITES);
+    new_object = array_builder.AllocateEmptyArray();
+  } else if (argument_count == 1) {
+    new_object = BuildInlinedCallNewArray_OneArg(expr, constructor,
+                                                 cell_instruction);
+  } else {
+    ASSERT(argument_count <= 5);

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

Why 5? Can we at least make it a descriptive constant somewhere?

[mvstanton, 2013/11/07 16:34:05]

Done.

+    JSArrayBuilder array_builder(this,
+                                 kind,
+                                 cell_instruction,
+                                 constructor,
+                                 DISABLE_ALLOCATION_SITES);
+    HValue* length = Add<HConstant>(argument_count);
+    bool fill_with_hole = IsFastSmiElementsKind(kind);
+    new_object = array_builder.AllocateArray(length, length, fill_with_hole);

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

Why do we only fill with holes if the kind is smi? At least add a comment.

[mvstanton, 2013/11/07 16:34:05]

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.

+    HValue* elements = array_builder.GetElementsLocation();
+    for (int i = 0; i < argument_count; i++) {
+      HValue* value = environment()->ExpressionStackAt(argument_count - i - 1);
+      HValue* constant_i = Add<HConstant>(i);
+      Add<HStoreKeyed>(elements, constant_i, value, kind);
+    }
+  }
+
+  Drop(argument_count + 1);  // drop constructor and args.
+  ast_context()->ReturnValue(new_object);
+}
+
+
 // Checks whether allocation using the given constructor can be inlined.
 static bool IsAllocationInlineable(Handle<JSFunction> constructor) {
   return constructor->has_initial_map() &&
@@ -7178,6 +7298,42 @@ static bool IsAllocationInlineable(Handle<JSFunction> constructor) {
 }
 
 
+bool HOptimizedGraphBuilder::IsCallNewArrayInlineable(CallNew* expr) {
+  bool inline_ok = false;
+  int argument_count = expr->arguments()->length();
+  // We should have the function plus array arguments on the environment stack.
+  ASSERT(environment()->length() >= (argument_count + 1));
+  if (argument_count <= 5) {

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

Why 5?

[mvstanton, 2013/11/07 16:34:05]

Why indeed. I eliminated this as any predicate.

+    Handle<Cell> cell = expr->allocation_info_cell();
+    AllocationSite* site = AllocationSite::cast(cell->value());
+    if (!site->DoNotInlineCall()) {

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

What about just CanInlineCall() rather than the double negative?

[mvstanton, 2013/11/07 16:34:05]

Done.

+      // We also want to avoid inlining in certain 1 argument scenarios.
+      if (argument_count == 1) {
+        HValue* argument = Top();
+        if (argument->IsConstant()) {
+          // Do not inline if the constant length argument is not a smi or
+          // outside the valid range for a fast array.
+          HConstant* constant_argument = HConstant::cast(argument);
+          if (constant_argument->HasSmiValue()) {
+            int value = constant_argument->Integer32Value();
+            inline_ok = value >= 0 &&
+                value < JSObject::kInitialMaxFastElementArray;
+          }
+        } else {
+          // The amount of code we'd have to generate doubles if we have a
+          // non-constant length, but packed elements_kind feedback. Don't
+          // inline this case.

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

Shouldn't we just generate a check whether the argument is 0 and bail out if
not?

[mvstanton, 2013/11/07 16:34:05]

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. 

+          inline_ok = IsHoleyElementsKind(expr->elements_kind());
+        }
+      } else {
+        inline_ok = true;
+      }
+    }
+  }
+  return inline_ok;
+}
+
+
 void HOptimizedGraphBuilder::VisitCallNew(CallNew* expr) {
   ASSERT(!HasStackOverflow());
   ASSERT(current_block() != NULL);
@@ -7186,14 +7342,15 @@ void HOptimizedGraphBuilder::VisitCallNew(CallNew* expr) {
   int argument_count = expr->arguments()->length() + 1;  // Plus constructor.
   Factory* factory = isolate()->factory();
 
+  // The constructor function is on the stack in the unoptimized code
+  // during evaluation of the arguments.
+  CHECK_ALIVE(VisitForValue(expr->expression()));
+  HValue* function = Top();
+  CHECK_ALIVE(VisitExpressions(expr->arguments()));
+
   if (FLAG_inline_construct &&
       expr->IsMonomorphic() &&
       IsAllocationInlineable(expr->target())) {
-    // The constructor function is on the stack in the unoptimized code
-    // during evaluation of the arguments.
-    CHECK_ALIVE(VisitForValue(expr->expression()));
-    HValue* function = Top();
-    CHECK_ALIVE(VisitExpressions(expr->arguments()));
     Handle<JSFunction> constructor = expr->target();
     HValue* check = Add<HCheckValue>(function, constructor);
 
@@ -7280,19 +7437,24 @@ void HOptimizedGraphBuilder::VisitCallNew(CallNew* expr) {
     // argument to the construct call.
     Handle<JSFunction> array_function(
         isolate()->global_context()->array_function(), isolate());
-    CHECK_ALIVE(VisitArgument(expr->expression()));
-    HValue* constructor = HPushArgument::cast(Top())->argument();

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

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.

[mvstanton, 2013/11/07 16:34:05]

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.

-    CHECK_ALIVE(VisitArgumentList(expr->arguments()));
+    bool use_call_new_array = expr->target().is_identical_to(array_function);
+    Handle<Cell> cell = expr->allocation_info_cell();
+    if (use_call_new_array && IsCallNewArrayInlineable(expr)) {
+      // Verify we are still calling the array function for our native context.
+      Add<HCheckValue>(function, array_function);
+      BuildInlinedCallNewArray(expr);
+      return;
+    }
+
     HBinaryCall* call;
-    if (expr->target().is_identical_to(array_function)) {
-      Handle<Cell> cell = expr->allocation_info_cell();
-      Add<HCheckValue>(constructor, array_function);
-      call = New<HCallNewArray>(constructor, argument_count,
-                                cell, expr->elements_kind());
+    if (use_call_new_array) {
+      Add<HCheckValue>(function, array_function);
+      call = New<HCallNewArray>(function, argument_count, cell,
+                                expr->elements_kind());
     } else {
-      call = New<HCallNew>(constructor, argument_count);
+      call = New<HCallNew>(function, argument_count);
     }
-    Drop(argument_count);
+    PreProcessCall(call);
     return ast_context()->ReturnInstruction(call, expr->id());
   }
 }