[Interpreter] Ensure we always have an outer register allocation scope.

Split RegisterAllocationScope out of ExpressionResult and allocate one
for each statement. This ensures that we always have an outer register
allocation scope for statement code (used in CountOperation and
RegisterExecutionResult). Also refactored the register allocator code to
move it to it's own file and rename from TemporaryRegisterScope to
BytecodeRegisterAllocator.

BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1587033002

Cr-Commit-Position: refs/heads/master@{#33296}

BUILD.gn

@@ -1087,6 +1087,8 @@ source_set("v8_base") {
     "src/interpreter/bytecode-array-iterator.h",
     "src/interpreter/bytecode-generator.cc",
     "src/interpreter/bytecode-generator.h",
+    "src/interpreter/bytecode-register-allocator.cc",
+    "src/interpreter/bytecode-register-allocator.h",
     "src/interpreter/bytecode-traits.h",
     "src/interpreter/constant-array-builder.cc",
     "src/interpreter/constant-array-builder.h",

src/interpreter/bytecode-array-builder.cc

@@ -1603,63 +1603,6 @@ bool BytecodeArrayBuilder::FitsInReg16Operand(Register value) {
   return kMinInt16 <= value.index() && value.index() <= kMaxInt16;
 }
 
-
-TemporaryRegisterScope::TemporaryRegisterScope(BytecodeArrayBuilder* builder)
-    : builder_(builder),
-      allocated_(builder->zone()),
-      next_consecutive_register_(-1),
-      next_consecutive_count_(-1) {}
-
-
-TemporaryRegisterScope::~TemporaryRegisterScope() {
-  for (auto i = allocated_.rbegin(); i != allocated_.rend(); i++) {
-    builder_->ReturnTemporaryRegister(*i);
-  }
-  allocated_.clear();
-}
-
-
-Register TemporaryRegisterScope::NewRegister() {
-  int allocated = -1;
-  if (next_consecutive_count_ <= 0) {
-    allocated = builder_->BorrowTemporaryRegister();
-  } else {
-    allocated = builder_->BorrowTemporaryRegisterNotInRange(
-        next_consecutive_register_,
-        next_consecutive_register_ + next_consecutive_count_ - 1);
-  }
-  allocated_.push_back(allocated);
-  return Register(allocated);
-}
-
-
-bool TemporaryRegisterScope::RegisterIsAllocatedInThisScope(
-    Register reg) const {
-  for (auto i = allocated_.begin(); i != allocated_.end(); i++) {
-    if (*i == reg.index()) return true;
-  }
-  return false;
-}
-
-
-void TemporaryRegisterScope::PrepareForConsecutiveAllocations(size_t count) {
-  if (static_cast<int>(count) > next_consecutive_count_) {
-    next_consecutive_register_ =
-        builder_->PrepareForConsecutiveTemporaryRegisters(count);
-    next_consecutive_count_ = static_cast<int>(count);
-  }
-}
-
-
-Register TemporaryRegisterScope::NextConsecutiveRegister() {
-  DCHECK_GE(next_consecutive_register_, 0);
-  DCHECK_GT(next_consecutive_count_, 0);
-  builder_->BorrowConsecutiveTemporaryRegister(next_consecutive_register_);
-  allocated_.push_back(next_consecutive_register_);
-  next_consecutive_count_--;
-  return Register(next_consecutive_register_++);
-}
-
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

src/interpreter/bytecode-array-builder.h

@@ -334,7 +334,8 @@ class BytecodeArrayBuilder final {
   ZoneSet<int> free_temporaries_;
 
   class PreviousBytecodeHelper;
-  friend class TemporaryRegisterScope;
+  friend class BytecodeRegisterAllocator;
+
   DISALLOW_COPY_AND_ASSIGN(BytecodeArrayBuilder);
 };
 
@@ -379,38 +380,6 @@ class BytecodeLabel final {
   friend class BytecodeArrayBuilder;
 };
 
-
-// A stack-allocated class than allows the instantiator to allocate
-// temporary registers that are cleaned up when scope is closed.
-// TODO(oth): Deprecate TemporaryRegisterScope use. Code should be
-// using result scopes as far as possible.
-class TemporaryRegisterScope {
- public:
-  explicit TemporaryRegisterScope(BytecodeArrayBuilder* builder);
-  ~TemporaryRegisterScope();
-  Register NewRegister();
-
-  void PrepareForConsecutiveAllocations(size_t count);
-  Register NextConsecutiveRegister();
-
-  bool RegisterIsAllocatedInThisScope(Register reg) const;
-
-  bool hasConsecutiveAllocations() const { return next_consecutive_count_ > 0; }
-
- private:
-  void* operator new(size_t size);
-  void operator delete(void* p);
-
-  BytecodeArrayBuilder* builder_;
-  const TemporaryRegisterScope* outer_;
-  ZoneVector<int> allocated_;
-  int next_consecutive_register_;
-  int next_consecutive_count_;
-
-  DISALLOW_COPY_AND_ASSIGN(TemporaryRegisterScope);
-};
-
-
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

src/interpreter/bytecode-generator.h

@@ -23,8 +23,9 @@ class BytecodeGenerator final : public AstVisitor {
   AST_NODE_LIST(DECLARE_VISIT)
 #undef DECLARE_VISIT
 
-  // Visiting function for declarations list is overridden.
+  // Visiting function for declarations list and statements are overridden.
   void VisitDeclarations(ZoneList<Declaration*>* declarations) override;
+  void VisitStatements(ZoneList<Statement*>* statments) override;
 
  private:
   class ContextScope;
@@ -35,6 +36,7 @@ class BytecodeGenerator final : public AstVisitor {
   class EffectResultScope;
   class AccumulatorResultScope;
   class RegisterResultScope;
+  class RegisterAllocationScope;
 
   void MakeBytecodeBody();
   Register NextContextRegister() const;
@@ -118,6 +120,13 @@ class BytecodeGenerator final : public AstVisitor {
     execution_result_ = execution_result;
   }
   ExpressionResultScope* execution_result() const { return execution_result_; }
+  inline void set_register_allocator(
+      RegisterAllocationScope* register_allocator) {
+    register_allocator_ = register_allocator;
+  }
+  RegisterAllocationScope* register_allocator() const {
+    return register_allocator_;
+  }
 
   ZoneVector<Handle<Object>>* globals() { return &globals_; }
   inline LanguageMode language_mode() const;
@@ -133,6 +142,7 @@ class BytecodeGenerator final : public AstVisitor {
   ControlScope* execution_control_;
   ContextScope* execution_context_;
   ExpressionResultScope* execution_result_;
+  RegisterAllocationScope* register_allocator_;
 };
 
 }  // namespace interpreter

src/interpreter/bytecode-register-allocator.cc

+// Copyright 2015 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/interpreter/bytecode-register-allocator.h"
+
+#include "src/interpreter/bytecode-array-builder.h"
+
+namespace v8 {
+namespace internal {
+namespace interpreter {
+
+BytecodeRegisterAllocator::BytecodeRegisterAllocator(
+    BytecodeArrayBuilder* builder)
+    : builder_(builder),
+      allocated_(builder->zone()),
+      next_consecutive_register_(-1),
+      next_consecutive_count_(-1) {}
+
+
+BytecodeRegisterAllocator::~BytecodeRegisterAllocator() {
+  for (auto i = allocated_.rbegin(); i != allocated_.rend(); i++) {
+    builder_->ReturnTemporaryRegister(*i);
+  }
+  allocated_.clear();
+}
+
+
+Register BytecodeRegisterAllocator::NewRegister() {
+  int allocated = -1;
+  if (next_consecutive_count_ <= 0) {
+    allocated = builder_->BorrowTemporaryRegister();
+  } else {
+    allocated = builder_->BorrowTemporaryRegisterNotInRange(
+        next_consecutive_register_,
+        next_consecutive_register_ + next_consecutive_count_ - 1);
+  }
+  allocated_.push_back(allocated);
+  return Register(allocated);
+}
+
+
+bool BytecodeRegisterAllocator::RegisterIsAllocatedInThisScope(
+    Register reg) const {
+  for (auto i = allocated_.begin(); i != allocated_.end(); i++) {
+    if (*i == reg.index()) return true;
+  }
+  return false;
+}
+
+
+void BytecodeRegisterAllocator::PrepareForConsecutiveAllocations(size_t count) {
+  if (static_cast<int>(count) > next_consecutive_count_) {
+    next_consecutive_register_ =
+        builder_->PrepareForConsecutiveTemporaryRegisters(count);
+    next_consecutive_count_ = static_cast<int>(count);
+  }
+}
+
+
+Register BytecodeRegisterAllocator::NextConsecutiveRegister() {
+  DCHECK_GE(next_consecutive_register_, 0);
+  DCHECK_GT(next_consecutive_count_, 0);
+  builder_->BorrowConsecutiveTemporaryRegister(next_consecutive_register_);
+  allocated_.push_back(next_consecutive_register_);
+  next_consecutive_count_--;
+  return Register(next_consecutive_register_++);
+}
+
+}  // namespace interpreter
+}  // namespace internal
+}  // namespace v8

src/interpreter/bytecode-register-allocator.h

+// Copyright 2015 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.
+
+#ifndef V8_INTERPRETER_BYTECODE_REGISTER_ALLOCATOR_H_
+#define V8_INTERPRETER_BYTECODE_REGISTER_ALLOCATOR_H_
+
+#include "src/zone-containers.h"
+
+namespace v8 {
+namespace internal {
+namespace interpreter {
+
+class BytecodeArrayBuilder;
+class Register;
+
+// A class than allows the instantiator to allocate temporary registers that are
+// cleaned up when scope is closed.
+class BytecodeRegisterAllocator {
+ public:
+  explicit BytecodeRegisterAllocator(BytecodeArrayBuilder* builder);
+  ~BytecodeRegisterAllocator();
+  Register NewRegister();
+
+  void PrepareForConsecutiveAllocations(size_t count);
+  Register NextConsecutiveRegister();
+
+  bool RegisterIsAllocatedInThisScope(Register reg) const;
+
+  bool HasConsecutiveAllocations() const { return next_consecutive_count_ > 0; }
+
+ private:
+  void* operator new(size_t size);
+  void operator delete(void* p);
+
+  BytecodeArrayBuilder* builder_;
+  ZoneVector<int> allocated_;
+  int next_consecutive_register_;
+  int next_consecutive_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(BytecodeRegisterAllocator);
+};
+
+}  // namespace interpreter
+}  // namespace internal
+}  // namespace v8
+
+
+#endif  // V8_INTERPRETER_BYTECODE_REGISTER_ALLOCATOR_H_

test/cctest/interpreter/test-bytecode-generator.cc

@@ -5317,7 +5317,7 @@ TEST(ForIn) {
        "  if (x['a'] == 10) continue;\n"
        "  if (x['a'] == 20) break;\n"
        "}",
-       9 * kPointerSize,
+       8 * kPointerSize,
        1,
        94,
        {
@@ -5344,17 +5344,17 @@ TEST(ForIn) {
            B(Ldar), R(0),                                                  //
            B(Star), R(6),                                                  //
            B(LoadICSloppy), R(6), U8(2), U8(vector->GetIndex(slot2)),      //
-           B(Star), R(8),                                                  //
+           B(Star), R(7),                                                  //
            B(LdaSmi8), U8(10),                                             //
-           B(TestEqual), R(8),                                             //
+           B(TestEqual), R(7),                                             //
            B(JumpIfFalse), U8(4),                                          //
            B(Jump), U8(20),                                                //
            B(Ldar), R(0),                                                  //
            B(Star), R(6),                                                  //
            B(LoadICSloppy), R(6), U8(2), U8(vector->GetIndex(slot3)),      //
-           B(Star), R(8),                                                  //
+           B(Star), R(7),                                                  //
            B(LdaSmi8), U8(20),                                             //
-           B(TestEqual), R(8),                                             //
+           B(TestEqual), R(7),                                             //
            B(JumpIfFalse), U8(4),                                          //
            B(Jump), U8(8),                                                 //
            B(ForInStep), R(5),                                             //

test/cctest/interpreter/test-interpreter.cc

@@ -2231,6 +2231,19 @@ TEST(InterpreterCountOperators) {
                      handle(Smi::FromInt(3), isolate)),
       std::make_pair("var a = 1; (function() { a = 2 })(); return a--;",
                      handle(Smi::FromInt(2), isolate)),
+      std::make_pair("var i = 5; while(i--) {}; return i;",
+                     handle(Smi::FromInt(-1), isolate)),
+      std::make_pair("var i = 1; if(i--) { return 1; } else { return 2; };",
+                     handle(Smi::FromInt(1), isolate)),
+      std::make_pair("var i = -2; do {} while(i++) {}; return i;",
+                      handle(Smi::FromInt(1), isolate)),
+      std::make_pair("var i = -1; for(; i++; ) {}; return i",
+                      handle(Smi::FromInt(1), isolate)),
+      std::make_pair("var i = 20; switch(i++) {\n"
+                     "  case 20: return 1;\n"
+                     "  default: return 2;\n"
+                     "}",
+                     handle(Smi::FromInt(1), isolate)),
   };
 
   for (size_t i = 0; i < arraysize(count_ops); i++) {

test/mjsunit/mjsunit.status

@@ -765,7 +765,6 @@
   'break': [SKIP],
   'call-runtime-tail': [SKIP],
   'compiler/compare-map-elim2': [SKIP],
-  'compiler/countoperation': [SKIP],
   'compiler/deopt-inlined-smi': [SKIP],
   'compiler/deopt-tonumber-compare': [SKIP],
   'compiler/escape-analysis-arguments': [SKIP],
@@ -782,8 +781,6 @@
   'compiler/optimize_min': [SKIP],
   'compiler/opt-next-call-turbo': [SKIP],
   'compiler/osr-forof': [SKIP],
-  'compiler/osr-manual2': [SKIP],
-  'compiler/phi-representations': [SKIP],
   'compiler/property-refs': [SKIP],
   'compiler/regress-3786': [SKIP],
   'compiler/regress-446647': [SKIP],
@@ -851,7 +848,6 @@
   'regress/regress-117409': [SKIP],
   'regress/regress-1177809': [SKIP],
   'regress/regress-119609': [SKIP],
-  'regress/regress-1209': [SKIP],
   'regress/regress-123919': [SKIP],
   'regress/regress-124594': [SKIP],
   'regress/regress-125515': [SKIP],
@@ -882,7 +878,6 @@
   'regress/regress-2318': [SKIP],
   'regress/regress-2339': [SKIP],
   'regress/regress-2374': [SKIP],
-  'regress/regress-2444': [SKIP],
   'regress/regress-2593': [SKIP],
   'regress/regress-2618': [SKIP],
   'regress/regress-263': [SKIP],
@@ -890,7 +885,6 @@
   'regress/regress-269': [SKIP],
   'regress/regress-2790': [SKIP],
   'regress/regress-2825': [SKIP],
-  'regress/regress-298269': [SKIP],
   'regress/regress-3135': [SKIP],
   'regress/regress-3138': [SKIP],
   'regress/regress-318420': [SKIP],

test/unittests/interpreter/bytecode-array-builder-unittest.cc

@@ -6,6 +6,7 @@
 
 #include "src/interpreter/bytecode-array-builder.h"
 #include "src/interpreter/bytecode-array-iterator.h"
+#include "src/interpreter/bytecode-register-allocator.h"
 #include "test/unittests/test-utils.h"
 
 namespace v8 {
@@ -317,7 +318,7 @@ TEST_F(BytecodeArrayBuilderTest, FrameSizesLookGood) {
         builder.set_locals_count(locals);
         builder.set_context_count(contexts);
 
-        TemporaryRegisterScope temporaries(&builder);
+        BytecodeRegisterAllocator temporaries(&builder);
         for (int i = 0; i < temps; i++) {
           builder.StoreAccumulatorInRegister(temporaries.NewRegister());
         }
@@ -332,32 +333,6 @@ TEST_F(BytecodeArrayBuilderTest, FrameSizesLookGood) {
 }
 
 
-TEST_F(BytecodeArrayBuilderTest, TemporariesRecycled) {
-  BytecodeArrayBuilder builder(isolate(), zone());
-  builder.set_parameter_count(0);
-  builder.set_locals_count(0);
-  builder.set_context_count(0);
-  builder.Return();
-
-  int first;
-  {
-    TemporaryRegisterScope temporaries(&builder);
-    first = temporaries.NewRegister().index();
-    temporaries.NewRegister();
-    temporaries.NewRegister();
-    temporaries.NewRegister();
-  }
-
-  int second;
-  {
-    TemporaryRegisterScope temporaries(&builder);
-    second = temporaries.NewRegister().index();
-  }
-
-  CHECK_EQ(first, second);
-}
-
-
 TEST_F(BytecodeArrayBuilderTest, RegisterValues) {
   int index = 1;
   uint8_t operand = static_cast<uint8_t>(-index);
@@ -391,15 +366,15 @@ TEST_F(BytecodeArrayBuilderTest, RegisterType) {
   builder.set_locals_count(3);
   builder.set_context_count(0);
 
-  TemporaryRegisterScope temporary_register_scope(&builder);
-  Register temp0 = temporary_register_scope.NewRegister();
+  BytecodeRegisterAllocator register_allocator(&builder);
+  Register temp0 = register_allocator.NewRegister();
   Register param0(builder.Parameter(0));
   Register param9(builder.Parameter(9));
-  Register temp1 = temporary_register_scope.NewRegister();
+  Register temp1 = register_allocator.NewRegister();
   Register reg0(0);
   Register reg1(1);
   Register reg2(2);
-  Register temp2 = temporary_register_scope.NewRegister();
+  Register temp2 = register_allocator.NewRegister();
   CHECK_EQ(builder.RegisterIsParameterOrLocal(temp0), false);
   CHECK_EQ(builder.RegisterIsParameterOrLocal(temp1), false);
   CHECK_EQ(builder.RegisterIsParameterOrLocal(temp2), false);

test/unittests/interpreter/bytecode-register-allocator-unittest.cc

+// Copyright 2014 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/v8.h"
+
+#include "src/interpreter/bytecode-array-builder.h"
+#include "src/interpreter/bytecode-register-allocator.h"
+#include "test/unittests/test-utils.h"
+
+namespace v8 {
+namespace internal {
+namespace interpreter {
+
+class BytecodeRegisterAllocatorTest : public TestWithIsolateAndZone {
+ public:
+  BytecodeRegisterAllocatorTest() {}
+  ~BytecodeRegisterAllocatorTest() override {}
+};
+
+
+TEST_F(BytecodeRegisterAllocatorTest, TemporariesRecycled) {
+  BytecodeArrayBuilder builder(isolate(), zone());
+  builder.set_parameter_count(0);
+  builder.set_locals_count(0);
+  builder.set_context_count(0);
+
+  int first;
+  {
+    BytecodeRegisterAllocator temporaries(&builder);
+    first = temporaries.NewRegister().index();
+    temporaries.NewRegister();
+    temporaries.NewRegister();
+    temporaries.NewRegister();
+  }
+
+  int second;
+  {
+    BytecodeRegisterAllocator temporaries(&builder);
+    second = temporaries.NewRegister().index();
+  }
+
+  CHECK_EQ(first, second);
+}
+
+
+TEST_F(BytecodeRegisterAllocatorTest, ConsecutiveRegisters) {
+  BytecodeArrayBuilder builder(isolate(), zone());
+  builder.set_parameter_count(0);
+  builder.set_locals_count(0);
+  builder.set_context_count(0);
+
+  BytecodeRegisterAllocator temporaries(&builder);
+  temporaries.PrepareForConsecutiveAllocations(4);
+  Register reg0 = temporaries.NextConsecutiveRegister();
+  Register other = temporaries.NewRegister();
+  Register reg1 = temporaries.NextConsecutiveRegister();
+  Register reg2 = temporaries.NextConsecutiveRegister();
+  Register reg3 = temporaries.NextConsecutiveRegister();
+  USE(other);
+
+  CHECK(Register::AreContiguous(reg0, reg1, reg2, reg3));
+}
+
+}  // namespace interpreter
+}  // namespace internal
+}  // namespace v8

test/unittests/unittests.gyp

@@ -97,6 +97,7 @@
         'interpreter/bytecodes-unittest.cc',
         'interpreter/bytecode-array-builder-unittest.cc',
         'interpreter/bytecode-array-iterator-unittest.cc',
+        'interpreter/bytecode-register-allocator-unittest.cc',
         'interpreter/constant-array-builder-unittest.cc',
         'libplatform/default-platform-unittest.cc',
         'libplatform/task-queue-unittest.cc',

tools/gyp/v8.gyp

@@ -863,6 +863,8 @@
         '../../src/interpreter/bytecode-array-builder.h',
         '../../src/interpreter/bytecode-array-iterator.cc',
         '../../src/interpreter/bytecode-array-iterator.h',
+        '../../src/interpreter/bytecode-register-allocator.cc',
+        '../../src/interpreter/bytecode-register-allocator.h',
         '../../src/interpreter/bytecode-generator.cc',
         '../../src/interpreter/bytecode-generator.h',
         '../../src/interpreter/bytecode-traits.h',