Do not bailout from optimizing functions that use f(x, arguments)

src/hydrogen.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/hydrogen.h"
 
 #include <sstream>
 
 #include "src/v8.h"
 
@@ skipping 4056 matching lines @@
 
 
 void EffectContext::ReturnValue(HValue* value) {
   // The value is simply ignored.
 }
 
 
 void ValueContext::ReturnValue(HValue* value) {
   // The value is tracked in the bailout environment, and communicated
   // through the environment as the result of the expression.
-  if (!arguments_allowed() && value->CheckFlag(HValue::kIsArguments)) {
-    owner()->Bailout(kBadValueContextForArgumentsValue);
+  if (value->CheckFlag(HValue::kIsArguments)) {
+    if (flag_ == ARGUMENTS_FAKED) {
+      value = owner()->graph()->GetConstantUndefined();
+    } else if (!arguments_allowed()) {
+      owner()->Bailout(kBadValueContextForArgumentsValue);
+    }
   }
   owner()->Push(value);
 }
 
 
 void TestContext::ReturnValue(HValue* value) {
   BuildBranch(value);
 }
 
 
@@ skipping 205 matching lines @@
 
 
 void HOptimizedGraphBuilder::VisitExpressions(
     ZoneList<Expression*>* exprs) {
   for (int i = 0; i < exprs->length(); ++i) {
     CHECK_ALIVE(VisitForValue(exprs->at(i)));
   }
 }
 
 
+void HOptimizedGraphBuilder::VisitExpressions(ZoneList<Expression*>* exprs,
+                                              ArgumentsAllowedFlag flag) {
+  for (int i = 0; i < exprs->length(); ++i) {
+    CHECK_ALIVE(VisitForValue(exprs->at(i), flag));
+  }
+}
+
+
 bool HOptimizedGraphBuilder::BuildGraph() {
   if (current_info()->function()->is_generator()) {
     Bailout(kFunctionIsAGenerator);
     return false;
   }
   Scope* scope = current_info()->scope();
   if (scope->HasIllegalRedeclaration()) {
     Bailout(kFunctionWithIllegalRedeclaration);
     return false;
   }
@@ skipping 4564 matching lines @@
     case kFunctionCall: {
       if (expr->arguments()->length() == 0) return false;
       BuildFunctionCall(expr);
       return true;
     }
     case kFunctionApply: {
       // For .apply, only the pattern f.apply(receiver, arguments)
       // is supported.
       if (current_info()->scope()->arguments() == NULL) return false;
 
-      ZoneList<Expression*>* args = expr->arguments();
-      if (args->length() != 2) return false;
+      if (!CanBeFunctionApplyArguments(expr, true)) return false;
 
-      VariableProxy* arg_two = args->at(1)->AsVariableProxy();
-      if (arg_two == NULL || !arg_two->var()->IsStackAllocated()) return false;
-      HValue* arg_two_value = LookupAndMakeLive(arg_two->var());
-      if (!arg_two_value->CheckFlag(HValue::kIsArguments)) return false;
       BuildFunctionApply(expr);
       return true;
     }
     default: { return false; }
   }
   UNREACHABLE();
 }
 
 
 void HOptimizedGraphBuilder::BuildFunctionApply(Call* expr) {
@@ skipping 266 matching lines @@
   }
 
   BuildArrayCall(expr,
                  expr->arguments()->length(),
                  function,
                  expr->allocation_site());
   return true;
 }
 
 
+bool HOptimizedGraphBuilder::CanBeFunctionApplyArguments(
+    Call* expr, bool mark_arguments_as_live) {
+  ZoneList<Expression*>* args = expr->arguments();
+  if (args->length() != 2) return false;
+  VariableProxy* arg_two = args->at(1)->AsVariableProxy();
+  if (arg_two == NULL || !arg_two->var()->IsStackAllocated()) return false;
+  HValue* arg_two_value = mark_arguments_as_live
+                              ? LookupAndMakeLive(arg_two->var())
+                              : Lookup(arg_two->var());
+  if (!arg_two_value->CheckFlag(HValue::kIsArguments)) return false;
+  return true;
+}
+
+
 void HOptimizedGraphBuilder::VisitCall(Call* expr) {
   DCHECK(!HasStackOverflow());
   DCHECK(current_block() != NULL);
   DCHECK(current_block()->HasPredecessor());
   Expression* callee = expr->expression();
   int argument_count = expr->arguments()->length() + 1;  // Plus receiver.
   HInstruction* call = NULL;
 
   Property* prop = callee->AsProperty();
   if (prop != NULL) {
@@ skipping 16 matching lines @@
     if (!prop->key()->IsPropertyName()) {
       CHECK_ALIVE(VisitForValue(prop->key()));
       key = Pop();
     }
 
     CHECK_ALIVE(PushLoad(prop, receiver, key));
     HValue* function = Pop();
 
     if (FLAG_hydrogen_track_positions) SetSourcePosition(expr->position());
 
-    // Push the function under the receiver.
-    environment()->SetExpressionStackAt(0, function);
 
-    Push(receiver);
 
     if (function->IsConstant() &&
         HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
+      // Push the function under the receiver.
+      environment()->SetExpressionStackAt(0, function);
+      Push(receiver);
+
       Handle<JSFunction> known_function = Handle<JSFunction>::cast(
           HConstant::cast(function)->handle(isolate()));
       expr->set_target(known_function);
 
       if (TryIndirectCall(expr)) return;
       CHECK_ALIVE(VisitExpressions(expr->arguments()));
 
       Handle<Map> map = types->length() == 1 ? types->first() : Handle<Map>();
       if (TryInlineBuiltinMethodCall(expr, known_function, map,
                                      expr->arguments()->length())) {
@@ skipping 15 matching lines @@
         // HWrapReceiver.
         call = New<HCallFunction>(
             function, argument_count, WRAP_AND_CALL);
       } else if (TryInlineCall(expr)) {
         return;
       } else {
         call = BuildCallConstantFunction(known_function, argument_count);
       }
 
     } else {
-      CHECK_ALIVE(VisitExpressions(expr->arguments()));
+      ArgumentsAllowedFlag arguments_flag = ARGUMENTS_NOT_ALLOWED;
+      if (CanBeFunctionApplyArguments(expr, false)) {

[Jakob Kummerow, 2014/11/19 20:46:16]

I'm not too happy with this condition. It lacks a check for not having type
feedback. As it is, *any* call of the form "some_function(something, arguments)"
will emit the deopt, as long as "some_function" isn't an HConstant. That's safe
in terms of functionality, but could hurt performance when it's part of a big,
expensive-to-optimize function where we'd try 10 times before giving up, rather
than disabling optimization immediately.

I don't see a good alternative right now ("expr->IsUninitialized()" doesn't
appear to be useful here); maybe we can find something tomorrow.

[Jakob Kummerow, 2014/11/20 09:59:15]

I've talked to mvstanton@, he suggested to implement something like:

bool TypeFeedbackOracle::CallIsUninitialized(FeedbackVectorICSlot slot) {
  return GetInfo(slot)->IsUndefined();
}

which can be queried with the slot returned by Call's CallFeedbackSlot().

[ulan, 2014/11/20 15:23:29]

Done.

[ulan, 2014/11/20 15:23:29]

Acknowledged.

+        Add<HDeoptimize>("Insufficient type feedback for call with arguments",
+                         Deoptimizer::EAGER);

[Jakob Kummerow, 2014/11/19 20:46:16]

If we find an appropriate check for "no type feedback yet", this should be
Deoptimizer::SOFT; otherwise it deserves a comment why it's EAGER.

[ulan, 2014/11/20 15:23:29]

Done.

+        arguments_flag = ARGUMENTS_FAKED;
+      }
+
+      // Push the function under the receiver.
+      environment()->SetExpressionStackAt(0, function);
+      Push(receiver);
+
+      CHECK_ALIVE(VisitExpressions(expr->arguments(), arguments_flag));
       CallFunctionFlags flags = receiver->type().IsJSObject()
           ? NO_CALL_FUNCTION_FLAGS : CALL_AS_METHOD;
       call = New<HCallFunction>(function, argument_count, flags);
     }
     PushArgumentsFromEnvironment(argument_count);
 
   } else {
     VariableProxy* proxy = expr->expression()->AsVariableProxy();
     if (proxy != NULL && proxy->var()->is_possibly_eval(isolate())) {
       return Bailout(kPossibleDirectCallToEval);
@@ skipping 3487 matching lines @@
   if (ShouldProduceTraceOutput()) {
     isolate()->GetHTracer()->TraceHydrogen(name(), graph_);
   }
 
 #ifdef DEBUG
   graph_->Verify(false);  // No full verify.
 #endif
 }
 
 } }  // namespace v8::internal