Optimize Function.prototype.call

src/hydrogen.cc

Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 3162eb606ad687924ae6bcd9ff04d0a1e9570869..47f303821e2047d238ff3bbad1ee13e1d06d2a68 100644
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -7978,11 +7978,35 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinFunctionCall(Call* expr) {
 bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(

[Toon Verwaest, 2014/06/12 21:41:13]

Does it make sense to have both methods? I'm pretty sure that by having both,
people will start calling the other version, which may not be correct?

[p.antonov, 2014/06/13 08:34:24]

Done.

     Call* expr,
     HValue* receiver,
-    Handle<Map> receiver_map) {
+    Handle<Map> map,
+    Handle<JSFunction> function,
+    int args_count_no_receiver
+) {

[Toon Verwaest, 2014/06/12 21:41:13]

Nit: put ) { back on the previous line

[p.antonov, 2014/06/13 08:34:24]

Done.

+  if (!function->shared()->HasBuiltinFunctionId()) return false;
+  BuiltinFunctionId id = function->shared()->builtin_function_id();
+  if (InlineBuiltinMethodCall(expr, receiver, map, id,
+                                args_count_no_receiver)) {
+    if (FLAG_trace_inlining) {
+      PrintF("Inlining builtin ");
+      function->ShortPrint();
+      PrintF("\n");
+    }
+    return true;
+  }
+  return false;
+}
+
+
+bool HOptimizedGraphBuilder::InlineBuiltinMethodCall(
+    Call* expr,
+    HValue* receiver,
+    Handle<Map> receiver_map,
+    BuiltinFunctionId id,
+    //  expr could be .call or .apply so it's not safe to rely on its

[Toon Verwaest, 2014/06/12 21:41:13]

I don't think we need this comment...

[p.antonov, 2014/06/13 08:34:24]

Done.

+    //  ->arguments()->length()
+    int args_count_no_receiver) {
+  int argument_count = args_count_no_receiver + 1;  // Plus receiver.
   // Try to inline calls like Math.* as operations in the calling function.
-  if (!expr->target()->shared()->HasBuiltinFunctionId()) return false;
-  BuiltinFunctionId id = expr->target()->shared()->builtin_function_id();
-  int argument_count = expr->arguments()->length() + 1;  // Plus receiver.
   switch (id) {
     case kStringCharCodeAt:
     case kStringCharAt:
@@ -8090,7 +8114,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       if (receiver_map->is_observed()) return false;
       ASSERT(receiver_map->is_extensible());
 
-      Drop(expr->arguments()->length());
+      Drop(args_count_no_receiver);
       HValue* result;
       HValue* reduced_length;
       HValue* receiver = Pop();
@@ -8166,7 +8190,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       Handle<JSObject> prototype(JSObject::cast(receiver_map->prototype()));
       BuildCheckPrototypeMaps(prototype, Handle<JSObject>());
 
-      const int argc = expr->arguments()->length();
+      const int argc = args_count_no_receiver;
       if (argc != 1) return false;
 
       HValue* value_to_push = Pop();
@@ -8223,7 +8247,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       // Threshold for fast inlined Array.shift().
       HConstant* inline_threshold = Add<HConstant>(static_cast<int32_t>(16));
 
-      Drop(expr->arguments()->length());
+      Drop(args_count_no_receiver);
       HValue* receiver = Pop();
       HValue* function = Pop();
       HValue* result;
@@ -8534,7 +8558,41 @@ bool HOptimizedGraphBuilder::TryInlineApiCall(Handle<JSFunction> function,
 }
 
 
-bool HOptimizedGraphBuilder::TryCallApply(Call* expr) {
+void HOptimizedGraphBuilder::HandleIndirectCall(Call* expr,
+                                                HValue* function,
+                                                int arguments_count) {
+  Handle<JSFunction> known_function;
+  int args_count_no_receiver = arguments_count - 1;
+  if (function->IsConstant() &&
+      HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
+    known_function = Handle<JSFunction>::cast(
+        HConstant::cast(function)->handle(isolate()));
+    if (TryInlineApply(known_function, expr, args_count_no_receiver)) {

[Toon Verwaest, 2014/06/12 21:41:13]

Seems like TryIndirectCall splits to apply and call handling, which join again
in HandleIndirectCall. It's a bit weird that it here again has special handling
for Apply. Is it really specific to Apply, or should it just be named
TryInlineIndirectCall?

Or perhaps we can even merge it with TryInlineCall since they don't behave that
different anymore.

[p.antonov, 2014/06/13 08:34:24]

Done.

+      return;
+    }
+
+    Handle<Map> map;
+    HValue* receiver = environment()->ExpressionStackAt(args_count_no_receiver);

[Toon Verwaest, 2014/06/12 21:41:13]

Can you move this code into TryInlineBuiltinMethodCall? 

[p.antonov, 2014/06/13 08:34:24]

Done.

+    if (receiver->IsConstant() &&
+        HConstant::cast(receiver)->handle(isolate())->IsHeapObject()) {
+      map = handle(Handle<HeapObject>::cast(
+          HConstant::cast(receiver)->handle(isolate()))->map());
+    }
+    if (TryInlineBuiltinMethodCall(expr, receiver, map, known_function,

[Toon Verwaest, 2014/06/12 21:41:13]

This was already done in BuildFunctionCallCall. Can we avoid this duplication?
Either pass in a flag for the extra behavior, or move this out to the Apply
variant. Well, I guess we first have to think about the extra behavior tout
court... see the comment down below.

[p.antonov, 2014/06/13 08:34:24]

Done.

+                                   args_count_no_receiver)) {
+      return;
+    }
+  }
+
+  PushArgumentsFromEnvironment(arguments_count);
+  HInvokeFunction* call = New<HInvokeFunction>(
+      function, known_function, arguments_count);
+  Drop(1);  // Function
+  ast_context()->ReturnInstruction(call, expr->id());
+}
+
+
+bool HOptimizedGraphBuilder::TryIndirectCall(Call* expr) {
   ASSERT(expr->expression()->IsProperty());
 
   if (!expr->IsMonomorphic()) {
@@ -8542,8 +8600,14 @@ bool HOptimizedGraphBuilder::TryCallApply(Call* expr) {
   }
   Handle<Map> function_map = expr->GetReceiverTypes()->first();
   if (function_map->instance_type() != JS_FUNCTION_TYPE ||
-      !expr->target()->shared()->HasBuiltinFunctionId() ||
-      expr->target()->shared()->builtin_function_id() != kFunctionApply) {
+      !expr->target()->shared()->HasBuiltinFunctionId()) {

[Toon Verwaest, 2014/06/12 21:41:13]

What about creating both BuildFunctionCall and BuildFunctionApply, and using a
switch to jump to those? I think you did that in your previous CL... That makes
it a bit more symmetrical.

[p.antonov, 2014/06/13 08:34:24]

Done.

+    return false;
+  }
+  if (expr->target()->shared()->builtin_function_id() == kFunctionCall) {
+    BuildFunctionCallCall(expr);
+    return true;
+  }
+  if (expr->target()->shared()->builtin_function_id() != kFunctionApply) {
     return false;
   }
 
@@ -8586,23 +8650,72 @@ bool HOptimizedGraphBuilder::TryCallApply(Call* expr) {
     for (int i = 1; i < arguments_count; i++) {
       Push(arguments_values->at(i));
     }
+    HandleIndirectCall(expr, function, arguments_count);
+    return true;
+  }
+}
 
-    Handle<JSFunction> known_function;
-    if (function->IsConstant() &&
-        HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
-      known_function = Handle<JSFunction>::cast(
+
+// f.call(...)
+void HOptimizedGraphBuilder::BuildFunctionCallCall(Call* expr) {
+  HValue* function = Pop();  // f
+  HValue* receiver;
+  ZoneList<Expression*>* args = expr->arguments();
+  int args_length = args->length();
+  bool is_builtin_call = false;
+  Handle<JSFunction> known_function;
+  Drop(1);  // call
+
+  if (function->IsConstant() &&
+      HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
+    known_function = Handle<JSFunction>::cast(
           HConstant::cast(function)->handle(isolate()));
-      int args_count = arguments_count - 1;  // Excluding receiver.
-      if (TryInlineApply(known_function, expr, args_count)) return true;
+    is_builtin_call = known_function->shared()->HasBuiltinFunctionId();
+  }
+
+  ArgumentsAllowedFlag flag = is_builtin_call
+      ? ARGUMENTS_ALLOWED : ARGUMENTS_NOT_ALLOWED;
+
+  if (args_length == 0) {
+    receiver = graph()->GetConstantUndefined();
+    args_length = 1;
+  } else {
+    CHECK_ALIVE(VisitForValue(args->at(0), flag));
+    receiver = Pop();
+  }
+  receiver = is_builtin_call ? receiver : BuildWrapReceiver(receiver, function);

[Toon Verwaest, 2014/06/12 21:41:14]

You can't just skip BuildWrapReceiver for builtins. BuildWrapReceiver has the
required logic to figure out whether it can be skipped or not (skipped only when
it's in strict-mode). Slice is in strict-mode, which is what you care about
most, so that should work out I presume?

[p.antonov, 2014/06/13 08:34:24]

Done.

+
+  Push(function);
+  Push(receiver);
+  for (int i = 1; i < args_length; i++) {
+    CHECK_ALIVE(VisitForValue(args->at(i), flag));
+  }
+
+  if (is_builtin_call) {
+    ASSERT(!known_function.is_null());
+
+    Handle<Map> map;
+    if (receiver->IsConstant() &&
+        HConstant::cast(receiver)->handle(isolate())->IsHeapObject()) {
+      map = handle(Handle<HeapObject>::cast(
+          HConstant::cast(receiver)->handle(isolate()))->map());
     }
 
-    PushArgumentsFromEnvironment(arguments_count);
-    HInvokeFunction* call = New<HInvokeFunction>(
-        function, known_function, arguments_count);
-    Drop(1);  // Function.
-    ast_context()->ReturnInstruction(call, expr->id());
-    return true;
+    int args_count_no_receiver = args_length - 1;
+    if (TryInlineBuiltinMethodCall(expr, receiver, map, known_function,
+                                   args_count_no_receiver)) {
+      return;
+    }
+    // Failing to inline as a builtin means arguments cannot
+    // be allowed after all.

[Toon Verwaest, 2014/06/12 21:41:13]

Iek! So what if I do the following?

a = [];
a.push(arguments);
return a;

[p.antonov, 2014/06/13 08:34:24]

Done.

+    for (int i = 0; i < args_length; ++i) {
+      HValue* arg = environment()->ExpressionStackAt(i);
+      if (arg->CheckFlag(HValue::kIsArguments)) {
+        return Bailout(kBadValueContextForArgumentsValue);
+      }
+    }
   }
+  HandleIndirectCall(expr, function, args_length);
 }
 
 
@@ -8867,16 +8980,12 @@ void HOptimizedGraphBuilder::VisitCall(Call* expr) {
           HConstant::cast(function)->handle(isolate()));
       expr->set_target(known_function);
 
-      if (TryCallApply(expr)) return;
+      if (TryIndirectCall(expr)) return;
       CHECK_ALIVE(VisitExpressions(expr->arguments()));
 
       Handle<Map> map = types->length() == 1 ? types->first() : Handle<Map>();
-      if (TryInlineBuiltinMethodCall(expr, receiver, map)) {
-        if (FLAG_trace_inlining) {
-          PrintF("Inlining builtin ");
-          known_function->ShortPrint();
-          PrintF("\n");
-        }
+      if (TryInlineBuiltinMethodCall(expr, receiver, map, known_function,
+                                     expr->arguments()->length())) {
         return;
       }
       if (TryInlineApiMethodCall(expr, receiver, types)) return;