[Interpreter] Allocates new temporary register outside the reservation for consecutive registers.

Consecutive registers are allocated in two passes. First we "reserve"
a set of registers and these get allocated when we actually use them.
If we request for a temporary register before we use all the consecutive
registers, the earlier implementation does not gaurantee that it allocates
outside the reservation for consecutive registers. This could cause problems
for example, in call_func(a, b++, c). This cl fixes
TemporaryRegisterScope::NewRegister, to return a new temporary register
outside the reservation for consecutive registers.

BUG=v8:4280
LOG=N

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

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

src/interpreter/bytecode-array-builder.cc

@@ -1034,6 +1034,34 @@ int BytecodeArrayBuilder::BorrowTemporaryRegister() {
 }
 
 
+int BytecodeArrayBuilder::BorrowTemporaryRegisterNotInRange(int start_index,
+                                                            int end_index) {
+  auto index = free_temporaries_.lower_bound(start_index);
+  if (index == free_temporaries_.begin()) {
+    // If start_index is the first free register, check for a register
+    // greater than end_index.
+    index = free_temporaries_.upper_bound(end_index);
+    if (index == free_temporaries_.end()) {
+      temporary_register_count_ += 1;
+      return last_temporary_register().index();
+    }
+  } else {
+    // If there is a free register < start_index
+    index--;
+  }
+
+  int retval = *index;
+  free_temporaries_.erase(index);
+  return retval;
+}
+
+
+int BytecodeArrayBuilder::AllocateAndBorrowTemporaryRegister() {
+  temporary_register_count_ += 1;
+  return last_temporary_register().index();
+}
+
+
 void BytecodeArrayBuilder::BorrowConsecutiveTemporaryRegister(int reg_index) {
   DCHECK(free_temporaries_.find(reg_index) != free_temporaries_.end());
   free_temporaries_.erase(reg_index);
@@ -1462,7 +1490,21 @@ TemporaryRegisterScope::~TemporaryRegisterScope() {
 
 
 Register TemporaryRegisterScope::NewRegister() {
-  int allocated = builder_->BorrowTemporaryRegister();
+  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);
+}
+
+
+Register TemporaryRegisterScope::AllocateNewRegister() {
+  int allocated = builder_->AllocateAndBorrowTemporaryRegister();
   allocated_.push_back(allocated);
   return Register(allocated);
 }

src/interpreter/bytecode-array-builder.h

@@ -278,6 +278,8 @@ class BytecodeArrayBuilder final {
   bool IsRegisterInAccumulator(Register reg);
 
   int BorrowTemporaryRegister();
+  int BorrowTemporaryRegisterNotInRange(int start_index, int end_index);
+  int AllocateAndBorrowTemporaryRegister();
   void ReturnTemporaryRegister(int reg_index);
   int PrepareForConsecutiveTemporaryRegisters(size_t count);
   void BorrowConsecutiveTemporaryRegister(int reg_index);
@@ -361,12 +363,15 @@ class TemporaryRegisterScope {
   explicit TemporaryRegisterScope(BytecodeArrayBuilder* builder);
   ~TemporaryRegisterScope();
   Register NewRegister();
+  Register AllocateNewRegister();
 
   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);

src/interpreter/bytecode-generator.cc

@@ -205,7 +205,22 @@ class BytecodeGenerator::ExpressionResultScope {
   BytecodeArrayBuilder* builder() const { return generator()->builder(); }
   ExpressionResultScope* outer() const { return outer_; }
 
-  Register NewRegister() { return allocator_.NewRegister(); }
+  Register NewRegister() {
+    ExpressionResultScope* current_scope = generator()->execution_result();
+    if ((current_scope == this) ||
+        (current_scope->outer() == this &&
+         !current_scope->allocator_.hasConsecutiveAllocations())) {
+      // Regular case - Allocating registers in current or outer context.
+      // VisitForRegisterValue allocates register in outer context.
+      return allocator_.NewRegister();
+    } else {
+      // We need this when allocating registers due to an Assignment hazard.
+      // It might be expensive to walk the full context chain and compute the
+      // list of consecutive reservations in the innerscopes. So allocates a
+      // new unallocated temporary register.
+      return allocator_.AllocateNewRegister();
+    }
+  }
 
   void PrepareForConsecutiveAllocations(size_t count) {
     allocator_.PrepareForConsecutiveAllocations(count);

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

@@ -5670,16 +5670,16 @@ TEST(AssignmentsInBinaryExpression) {
            B(Add), R(4),        //
            B(Star), R(3),       //
            B(LdaSmi8), U8(4),   //
-           B(Star), R(4),       //
-           B(Add), R(3),        //
            B(Star), R(5),       //
+           B(Add), R(3),        //
+           B(Star), R(4),       //
            B(LdaSmi8), U8(5),   //
            B(Star), R(1),       //
-           B(Add), R(5),        //
+           B(Add), R(4),        //
            B(Star), R(3),       //
            B(Ldar), R(1),       //
            B(Add), R(3),        //
-           B(Mov), R(4), R(0),  //
+           B(Mov), R(5), R(0),  //
            B(Return),           //
        },
        0},

test/cctest/interpreter/test-interpreter.cc

@@ -3129,6 +3129,40 @@ TEST(InterpreterLookupSlot) {
   }
 }
 
+
+TEST(TemporaryRegisterAllocation) {
+  HandleAndZoneScope handles;
+  i::Isolate* isolate = handles.main_isolate();
+  i::Factory* factory = isolate->factory();
+
+  std::pair<const char*, Handle<Object>> reg_tests[] = {
+      {"function add(a, b, c) {"
+       "   return a + b + c;"
+       "}"
+       "function f() {"
+       "  var a = 10, b = 10;"
+       "   return add(a, b++, b);"
+       "}",
+       factory->NewNumberFromInt(31)},
+      {"function add(a, b, c, d) {"
+       "  return a + b + c + d;"
+       "}"
+       "function f() {"
+       "  var x = 10, y = 20, z = 30;"
+       "  return x + add(x, (y= x++), x, z);"
+       "}",
+       factory->NewNumberFromInt(71)},
+  };
+
+  for (size_t i = 0; i < arraysize(reg_tests); i++) {
+    InterpreterTester tester(handles.main_isolate(), reg_tests[i].first);
+    auto callable = tester.GetCallable<>();
+
+    Handle<i::Object> return_value = callable().ToHandleChecked();
+    CHECK(return_value->SameValue(*reg_tests[i].second));
+  }
+}
+
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

test/mjsunit/mjsunit.status

@@ -782,7 +782,6 @@
   'double-equals': [SKIP],
   'double-intrinsics': [SKIP],
   'elements-transition-hoisting': [SKIP],
-  'error-constructors': [SKIP],
   'eval-enclosing-function-name': [SKIP],
   'eval-stack-trace': [SKIP],
   'fast-prototype': [SKIP],
@@ -834,7 +833,6 @@
   'regress/regress-1369': [SKIP],
   'regress/regress-1403': [SKIP],
   'regress/regress-1412': [SKIP],
-  'regress/regress-1415': [SKIP],
   'regress/regress-1436': [SKIP],
   'regress/regress-1493017': [SKIP],
   'regress/regress-1523': [SKIP],
@@ -855,7 +853,6 @@
   'regress/regress-2339': [SKIP],
   'regress/regress-2374': [SKIP],
   'regress/regress-2444': [SKIP],
-  'regress/regress-244': [SKIP],
   'regress/regress-2593': [SKIP],
   'regress/regress-2594': [SKIP],
   'regress/regress-2618': [SKIP],
@@ -954,7 +951,6 @@
   'regress/regress-crbug-242502': [SKIP],
   'regress/regress-crbug-242924': [SKIP],
   'regress/regress-crbug-245480': [SKIP],
-  'regress/regress-crbug-349079': [SKIP],
   'regress/regress-crbug-350864': [SKIP],
   'regress/regress-crbug-351262': [SKIP],
   'regress/regress-crbug-352058': [SKIP],
@@ -1000,7 +996,6 @@
   'regress/regress-embedded-cons-string': [SKIP],
   'regress/regress-existing-shared-function-info': [SKIP],
   'regress/regress-fast-literal-transition': [SKIP],
-  'regress/regress-force-representation': [SKIP],
   'regress/regress-function-constructor-receiver': [SKIP],
   'regress/regress-handle-illegal-redeclaration': [SKIP],
   'regress/regress-inline-class-constructor': [SKIP],
@@ -1041,7 +1036,6 @@
   'unary-minus-deopt': [SKIP],
   'undetectable-compare': [SKIP],
   'unused-context-in-with': [SKIP],
-  'uri': [SKIP],
   'value-wrapper': [SKIP],
   'with-parameter-access': [SKIP],
   'with-prototype': [SKIP],

test/test262/test262.status

@@ -614,7 +614,6 @@
   'annexB/B.2.3.*': [SKIP],
   'built-ins/Array/prototype/reduce/*': [SKIP],
   'built-ins/Array/prototype/reduceRight/*': [SKIP],
-  'built-ins/decodeURI*': [SKIP],
   'built-ins/GeneratorFunction/*': [SKIP],
   'built-ins/GeneratorPrototype/*': [SKIP],
   'built-ins/Map/*': [SKIP],