Remove ARGUMENTS_VARIABLE and fix crankshaft to properly detect the arguments...

Remove ARGUMENTS_VARIABLE and fix crankshaft to properly detect the arguments object and keep it alive when inlining .apply

BUG=

Review-Url: https://codereview.chromium.org/2367483003
Cr-Commit-Position: refs/heads/master@{#39670}

src/ast/scopes.cc

@@ -586,21 +586,17 @@ void DeclarationScope::DeclareArguments(AstValueFactory* ast_value_factory) {
   DCHECK(is_function_scope());
   DCHECK(!is_arrow_scope());
 
-  // Check if there's lexically declared variable named arguments to avoid
-  // redeclaration. See ES#sec-functiondeclarationinstantiation, step 20.
-  Variable* arg_variable = LookupLocal(ast_value_factory->arguments_string());
-  if (arg_variable != nullptr && IsLexicalVariableMode(arg_variable->mode())) {
-    return;
-  }
-
-  // Declare 'arguments' variable which exists in all non arrow functions.
-  // Note that it might never be accessed, in which case it won't be
-  // allocated during variable allocation.
-  if (arg_variable == nullptr) {
+  arguments_ = LookupLocal(ast_value_factory->arguments_string());
+  if (arguments_ == nullptr) {
+    // Declare 'arguments' variable which exists in all non arrow functions.
+    // Note that it might never be accessed, in which case it won't be
+    // allocated during variable allocation.
     arguments_ = Declare(zone(), this, ast_value_factory->arguments_string(),
-                         VAR, ARGUMENTS_VARIABLE, kCreatedInitialized);
-  } else {
-    arguments_ = arg_variable;
+                         VAR, NORMAL_VARIABLE, kCreatedInitialized);
+  } else if (IsLexicalVariableMode(arguments_->mode())) {
+    // Check if there's lexically declared variable named arguments to avoid
+    // redeclaration. See ES#sec-functiondeclarationinstantiation, step 20.
+    arguments_ = nullptr;
   }
 }
 

src/ast/variables.h

@@ -78,7 +78,6 @@ class Variable final : public ZoneObject {
 
   bool is_function() const { return kind() == FUNCTION_VARIABLE; }
   bool is_this() const { return kind() == THIS_VARIABLE; }
-  bool is_arguments() const { return kind() == ARGUMENTS_VARIABLE; }
   bool is_sloppy_function_name() const {
     return kind() == SLOPPY_FUNCTION_NAME_VARIABLE;
   }

src/bailout-reason.h

@@ -20,7 +20,6 @@ namespace internal {
   V(kArgumentsObjectValueInATestContext,                                       \
     "Arguments object value in a test context")                                \
   V(kArrayIndexConstantValueTooBig, "Array index constant value too big")      \
-  V(kAssignmentToArguments, "Assignment to arguments")                         \
   V(kAssignmentToLetVariableBeforeInitialization,                              \
     "Assignment to let variable before initialization")                        \
   V(kAssignmentToLOOKUPVariable, "Assignment to LOOKUP variable")              \

src/crankshaft/hydrogen.cc

@@ -6934,8 +6934,6 @@ void HOptimizedGraphBuilder::VisitAssignment(Assignment* expr) {
       }
     }
 
-    if (var->is_arguments()) return Bailout(kAssignmentToArguments);
-
     // Handle the assignment.
     switch (var->location()) {
       case VariableLocation::UNALLOCATED:
@@ -9314,8 +9312,6 @@ bool HOptimizedGraphBuilder::TryIndirectCall(Call* expr) {
     case kFunctionApply: {
       // For .apply, only the pattern f.apply(receiver, arguments)
       // is supported.
-      if (current_info()->scope()->arguments() == NULL) return false;
-
       if (!CanBeFunctionApplyArguments(expr)) return false;
 
       BuildFunctionApply(expr);
@@ -9335,6 +9331,10 @@ void HOptimizedGraphBuilder::BuildFunctionApply(Call* expr) {
   HValue* function = Pop();  // f
   Drop(1);  // apply
 
+  // Make sure the arguments object is live.
+  VariableProxy* arg_two = args->at(1)->AsVariableProxy();
+  LookupAndMakeLive(arg_two->var());
+
   Handle<Map> function_map = expr->GetReceiverTypes()->first();
   HValue* checked_function = AddCheckMap(function, function_map);
 
@@ -9580,8 +9580,9 @@ bool HOptimizedGraphBuilder::CanBeFunctionApplyArguments(Call* expr) {
   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 = LookupAndMakeLive(arg_two->var());
+  HValue* arg_two_value = environment()->Lookup(arg_two->var());
   if (!arg_two_value->CheckFlag(HValue::kIsArguments)) return false;
+  DCHECK_NOT_NULL(current_info()->scope()->arguments());
   return true;
 }
 

src/crankshaft/hydrogen.h

@@ -2300,11 +2300,9 @@ class HOptimizedGraphBuilder : public HGraphBuilder,
                                                 int index,
                                                 HEnvironment* env) {
     if (!FLAG_analyze_environment_liveness) return false;
-    // |this| and |arguments| are always live; zapping parameters isn't
-    // safe because function.arguments can inspect them at any time.
-    return !var->is_this() &&
-           !var->is_arguments() &&
-           env->is_local_index(index);
+    // Zapping parameters isn't safe because function.arguments can inspect them
+    // at any time.
+    return env->is_local_index(index);
   }
   void BindIfLive(Variable* var, HValue* value) {
     HEnvironment* env = environment();

src/globals.h

@@ -970,7 +970,6 @@ enum VariableKind : uint8_t {
   NORMAL_VARIABLE,
   FUNCTION_VARIABLE,
   THIS_VARIABLE,
-  ARGUMENTS_VARIABLE,
   SLOPPY_FUNCTION_NAME_VARIABLE,
   kLastKind = SLOPPY_FUNCTION_NAME_VARIABLE
 };

test/cctest/interpreter/bytecode_expectations/Generators.golden

@@ -23,7 +23,7 @@ bytecodes: [
                 B(LdaZero),
                 B(TestEqualStrict), R(1), U8(0),
                 B(JumpIfTrue), U8(61),
-                B(LdaSmi), U8(77),
+                B(LdaSmi), U8(76),
                 B(Star), R(2),
                 B(CallRuntime), U16(Runtime::kAbort), R(2), U8(1),
                 B(LdaSmi), U8(-2),
@@ -131,7 +131,7 @@ bytecodes: [
                 B(LdaSmi), U8(1),
                 B(TestEqualStrict), R(1), U8(0),
                 B(JumpIfTrueConstant), U8(0),
-                B(LdaSmi), U8(77),
+                B(LdaSmi), U8(76),
                 B(Star), R(2),
                 B(CallRuntime), U16(Runtime::kAbort), R(2), U8(1),
                 B(LdaSmi), U8(-2),
@@ -279,7 +279,7 @@ bytecodes: [
                 B(LdaSmi), U8(1),
                 B(TestEqualStrict), R(4), U8(0),
                 B(JumpIfTrueConstant), U8(3),
-                B(LdaSmi), U8(77),
+                B(LdaSmi), U8(76),
                 B(Star), R(5),
                 B(CallRuntime), U16(Runtime::kAbort), R(5), U8(1),
                 B(LdaSmi), U8(-2),
@@ -345,7 +345,7 @@ bytecodes: [
                 B(LdaSmi), U8(1),
                 B(TestEqualStrict), R(4), U8(0),
                 B(JumpIfTrueConstant), U8(9),
-                B(LdaSmi), U8(77),
+                B(LdaSmi), U8(76),
                 B(Star), R(12),
                 B(CallRuntime), U16(Runtime::kAbort), R(12), U8(1),
   /*   27 S> */ B(LdrContextSlot), R(1), U8(7), U8(0), R(14),

test/mjsunit/regress/regress-arguments-liveness-analysis.js

+// Copyright 2016 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.
+
+// Flags: --allow-natives-syntax
+
+function r(v) { return v.f }
+function h() { }
+function y(v) {
+  var x = arguments;
+  h.apply(r(v), x);
+};
+
+y({f:3});
+y({f:3});
+y({f:3});
+
+%OptimizeFunctionOnNextCall(y);
+
+y({ f : 3, u : 4 });