[Interpreter] Reduce temporary register usage in generated bytecode.

src/interpreter/bytecode-generator.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-generator.h"
 
-#include <stack>
-
 #include "src/compiler.h"
 #include "src/interpreter/control-flow-builders.h"
 #include "src/objects.h"
 #include "src/parser.h"
 #include "src/scopes.h"
 #include "src/token.h"
 
 namespace v8 {
 namespace internal {
 namespace interpreter {
@@ skipping 120 matching lines @@
                                                      Statement* statement) {
   ControlScope* current = this;
   do {
     if (current->Execute(command, statement)) return;
     current = current->outer();
   } while (current != nullptr);
   UNREACHABLE();
 }
 
 
+// Scoped base class for determining where the result of an expression
+// is stored.
+class BytecodeGenerator::ExpressionResultScope {
+ public:
+  explicit ExpressionResultScope(BytecodeGenerator* generator)
+      : generator_(generator),
+        outer_(generator->result_scope()),
+        allocator_(builder()),
+        result_identified_(false) {
+    generator_->set_result_scope(this);
+  }
+
+  virtual ~ExpressionResultScope() {
+    generator_->set_result_scope(outer_);
+    DCHECK_EQ(result_identified(), true);
+  }
+
+  virtual void SetResultInAccumulator() = 0;
+  virtual void SetResultInRegister(Register reg) = 0;
+
+  BytecodeGenerator* generator() const { return generator_; }
+  BytecodeArrayBuilder* builder() const { return generator()->builder(); }
+  ExpressionResultScope* outer() const { return outer_; }
+
+  Register NewRegister() { return allocator_.NewRegister(); }
+
+  void PrepareForConsecutiveAllocations(size_t count) {
+    allocator_.PrepareForConsecutiveAllocations(count);
+  }
+
+  Register NextConsecutiveRegister() {
+    return allocator_.NextConsecutiveRegister();
+  }
+
+ protected:
+  void set_result_identified() {
+    DCHECK(!result_identified());
+    result_identified_ = true;
+  }
+
+  bool result_identified() const { return result_identified_; }
+
+ private:
+  BytecodeGenerator* generator_;
+  ExpressionResultScope* outer_;
+  TemporaryRegisterScope allocator_;
+  bool result_identified_;
+  DISALLOW_COPY_AND_ASSIGN(ExpressionResultScope);
+};
+
+
+// Scoped class used when the result of the current expression is not
+// expected to produce a result.
+class BytecodeGenerator::EffectResultScope final
+    : public ExpressionResultScope {
+ public:
+  explicit EffectResultScope(BytecodeGenerator* generator)
+      : ExpressionResultScope(generator) {
+    set_result_identified();
+  }
+
+  virtual void SetResultInAccumulator() {}
+  virtual void SetResultInRegister(Register reg) {}
+};
+
+
+// Scoped class used when the result of the current expression to be
+// evaluated should go into the interpreter's accumulator register.
+class BytecodeGenerator::AccumulatorResultScope final
+    : public ExpressionResultScope {
+ public:
+  explicit AccumulatorResultScope(BytecodeGenerator* generator)
+      : ExpressionResultScope(generator) {}
+
+  virtual void SetResultInAccumulator() { set_result_identified(); }
+
+  virtual void SetResultInRegister(Register reg) {
+    builder()->LoadAccumulatorWithRegister(reg);
+    set_result_identified();
+  }
+};
+
+
+// Scoped class used when the result of the current expression to be
+// evaluated should go into an interpreter register.
+class BytecodeGenerator::RegisterResultScope final
+    : public ExpressionResultScope {
+ public:
+  explicit RegisterResultScope(BytecodeGenerator* generator)
+      : ExpressionResultScope(generator) {}
+  ~RegisterResultScope() {}
+
+  virtual void SetResultInAccumulator() {
+    result_register_ = outer()->NewRegister();
+    builder()->StoreAccumulatorInRegister(result_register_);
+    set_result_identified();
+  }
+
+  virtual void SetResultInRegister(Register reg) {
+    DCHECK(builder()->RegisterIsParameterOrLocal(reg) ||
+           builder()->RegisterIsTemporary(reg));
+    result_register_ = reg;
+    set_result_identified();
+  }
+
+  Register ResultRegister() const { return result_register_; }
+
+ private:
+  Register result_register_;
+};
+
+
 BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
     : isolate_(isolate),
       zone_(zone),
       builder_(isolate, zone),
       info_(nullptr),
       scope_(nullptr),
       globals_(0, zone),
       execution_control_(nullptr),
-      execution_context_(nullptr) {
+      execution_context_(nullptr),
+      result_scope_(nullptr),
+      binary_expression_depth_(0),
+      binary_expression_hazard_set_(zone) {
   InitializeAstVisitor(isolate);
 }
 
 
 BytecodeGenerator::~BytecodeGenerator() {}
 
 
 Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   set_info(info);
   set_scope(info->scope());
@@ skipping 149 matching lines @@
   builder()->LoadLiteral(Smi::FromInt(encoded_flags));
   builder()->StoreAccumulatorInRegister(flags);
   DCHECK(flags.index() == pairs.index() + 1);
 
   builder()->CallRuntime(Runtime::kDeclareGlobals, pairs, 2);
   globals()->clear();
 }
 
 
 void BytecodeGenerator::VisitExpressionStatement(ExpressionStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForEffect(stmt->expression());
 }
 
 
 void BytecodeGenerator::VisitEmptyStatement(EmptyStatement* stmt) {
-  // TODO(oth): For control-flow it could be useful to signal empty paths here.
 }
 
 
 void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
   // TODO(oth): Spot easy cases where there code would not need to
   // emit the then block or the else block, e.g. condition is
   // obviously true/1/false/0.
 
   BytecodeLabel else_label, end_label;
 
-  Visit(stmt->condition());
+  VisitForAccumulatorValue(stmt->condition());
   builder()->CastAccumulatorToBoolean();
   builder()->JumpIfFalse(&else_label);
   Visit(stmt->then_statement());
   if (stmt->HasElseStatement()) {
     builder()->Jump(&end_label);
     builder()->Bind(&else_label);
     Visit(stmt->else_statement());
   } else {
     builder()->Bind(&else_label);
   }
@@ skipping 11 matching lines @@
   execution_control()->Continue(stmt->target());
 }
 
 
 void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
   execution_control()->Break(stmt->target());
 }
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitSwitchStatement(SwitchStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitCaseClause(CaseClause* clause) { UNIMPLEMENTED(); }
 
 
 void BytecodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
 
   BytecodeLabel body_label, condition_label, done_label;
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&condition_label);
-  Visit(stmt->cond());
+  VisitForAccumulatorValue(stmt->cond());
   builder()->JumpIfTrue(&body_label);
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(condition_label);
 }
 
 
 void BytecodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
 
   BytecodeLabel body_label, condition_label, done_label;
   builder()->Jump(&condition_label);
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&condition_label);
-  Visit(stmt->cond());
+  VisitForAccumulatorValue(stmt->cond());
   builder()->JumpIfTrue(&body_label);
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(condition_label);
 }
 
 
 void BytecodeGenerator::VisitForStatement(ForStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
 
   if (stmt->init() != nullptr) {
     Visit(stmt->init());
   }
 
   BytecodeLabel body_label, condition_label, next_label, done_label;
   if (stmt->cond() != nullptr) {
     builder()->Jump(&condition_label);
   }
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&next_label);
   if (stmt->next() != nullptr) {
     Visit(stmt->next());
   }
   if (stmt->cond()) {
     builder()->Bind(&condition_label);
-    Visit(stmt->cond());
+    VisitForAccumulatorValue(stmt->cond());
     builder()->JumpIfTrue(&body_label);
   } else {
     builder()->Jump(&body_label);
   }
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(next_label);
 }
 
@@ skipping 34 matching lines @@
 
 void BytecodeGenerator::VisitFunctionLiteral(FunctionLiteral* expr) {
   // Find or build a shared function info.
   Handle<SharedFunctionInfo> shared_info =
       Compiler::GetSharedFunctionInfo(expr, info()->script(), info());
   CHECK(!shared_info.is_null());  // TODO(rmcilroy): Set stack overflow?
 
   builder()
       ->LoadLiteral(shared_info)
       .CreateClosure(expr->pretenure() ? TENURED : NOT_TENURED);
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitClassLiteral(ClassLiteral* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitNativeFunctionLiteral(
     NativeFunctionLiteral* expr) {
@@ skipping 14 matching lines @@
     builder()->LoadTrue();
   } else if (value->IsFalse()) {
     builder()->LoadFalse();
   } else if (value->IsNull()) {
     builder()->LoadNull();
   } else if (value->IsTheHole()) {
     builder()->LoadTheHole();
   } else {
     builder()->LoadLiteral(value);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
   // Materialize a regular expression literal.
   TemporaryRegisterScope temporary_register_scope(builder());
   Register flags = temporary_register_scope.NewRegister();
   builder()
       ->LoadLiteral(expr->flags())
       .StoreAccumulatorInRegister(flags)
       .LoadLiteral(expr->pattern())
       .CreateRegExpLiteral(expr->literal_index(), flags);
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   // Deep-copy the literal boilerplate.
   builder()
       ->LoadLiteral(expr->constant_properties())
       .CreateObjectLiteral(expr->literal_index(), expr->ComputeFlags(true));
 
   TemporaryRegisterScope temporary_register_scope(builder());
@@ skipping 24 matching lines @@
       // Fall through.
       case ObjectLiteral::Property::COMPUTED: {
         // It is safe to use [[Put]] here because the boilerplate already
         // contains computed properties with an uninitialized value.
         if (literal_key->value()->IsInternalizedString()) {
           if (property->emit_store()) {
             Register name = inner_temporary_register_scope.NewRegister();
             builder()
                 ->LoadLiteral(literal_key->AsPropertyName())
                 .StoreAccumulatorInRegister(name);
-            Visit(property->value());
+            VisitForAccumulatorValue(property->value());
             builder()->StoreNamedProperty(literal, name,
                                           feedback_index(property->GetSlot(0)),
                                           language_mode());
           } else {
-            Visit(property->value());
+            VisitForEffect(property->value());
           }
         } else {
-          Register key = inner_temporary_register_scope.NewRegister();
-          Register value = inner_temporary_register_scope.NewRegister();
-          Register language = inner_temporary_register_scope.NewRegister();
+          inner_temporary_register_scope.PrepareForConsecutiveAllocations(3);
+          Register key =
+              inner_temporary_register_scope.NextConsecutiveRegister();
+          Register value =
+              inner_temporary_register_scope.NextConsecutiveRegister();
+          Register language =
+              inner_temporary_register_scope.NextConsecutiveRegister();
+          // TODO(oth): This is problematic - can't assume contiguous here.
+          // literal is allocated in temporary_register_scope, whereas
+          // key, value, language are in another.

[rmcilroy, 2015/10/20 16:39:20]

Hmm, this is tricky. One option would be to do the
PrepareForConsecutiveAllocations() on the outer temporary_register_scope, and
declare key, value, language outside the for loop and allocated them if they are
invalid here (same for all the other uses of NextConsecutiveRegister in this
function). It's a bit messy though. Any other ideas? Fine to leave this as a
TODO for now.

[oth, 2015/10/20 20:58:42]

We may have exactly the right mix of allocation scopes for this to work just
now. It's a little hairy.

Another alternative would be to include an additional allocation that could be
guaranteed to be consecutive and move the literal register into this, e.g.
Builder()->mov(literal_consecutive, literal). It burns a register, but is safe.

           DCHECK(Register::AreContiguous(literal, key, value, language));
-          Visit(property->key());
+          VisitForAccumulatorValue(property->key());
           builder()->StoreAccumulatorInRegister(key);
-          Visit(property->value());
+          VisitForAccumulatorValue(property->value());
           builder()->StoreAccumulatorInRegister(value);
           if (property->emit_store()) {
             builder()
                 ->LoadLiteral(Smi::FromInt(SLOPPY))
                 .StoreAccumulatorInRegister(language)
                 .CallRuntime(Runtime::kSetProperty, literal, 4);
             VisitSetHomeObject(value, literal, property);
           }
         }
         break;
       }
       case ObjectLiteral::Property::PROTOTYPE: {
+        inner_temporary_register_scope.PrepareForConsecutiveAllocations(1);
         DCHECK(property->emit_store());
-        Register value = inner_temporary_register_scope.NewRegister();
+        Register value =
+            inner_temporary_register_scope.NextConsecutiveRegister();
         DCHECK(Register::AreContiguous(literal, value));
-        Visit(property->value());
+        VisitForAccumulatorValue(property->value());
         builder()->StoreAccumulatorInRegister(value).CallRuntime(
             Runtime::kInternalSetPrototype, literal, 2);
         break;
       }
       case ObjectLiteral::Property::GETTER:
         if (property->emit_store()) {
           accessor_table.lookup(literal_key)->second->getter = property;
         }
         break;
       case ObjectLiteral::Property::SETTER:
         if (property->emit_store()) {
           accessor_table.lookup(literal_key)->second->setter = property;
         }
         break;
     }
   }
 
   // Define accessors, using only a single call to the runtime for each pair of
   // corresponding getters and setters.
   for (AccessorTable::Iterator it = accessor_table.begin();
        it != accessor_table.end(); ++it) {
     TemporaryRegisterScope inner_temporary_register_scope(builder());
-    Register name = inner_temporary_register_scope.NewRegister();
-    Register getter = inner_temporary_register_scope.NewRegister();
-    Register setter = inner_temporary_register_scope.NewRegister();
-    Register attr = inner_temporary_register_scope.NewRegister();
+    inner_temporary_register_scope.PrepareForConsecutiveAllocations(4);
+    Register name = inner_temporary_register_scope.NextConsecutiveRegister();
+    Register getter = inner_temporary_register_scope.NextConsecutiveRegister();
+    Register setter = inner_temporary_register_scope.NextConsecutiveRegister();
+    Register attr = inner_temporary_register_scope.NextConsecutiveRegister();
     DCHECK(Register::AreContiguous(literal, name, getter, setter, attr));
-    Visit(it->first);
+    VisitForAccumulatorValue(it->first);
     builder()->StoreAccumulatorInRegister(name);
     VisitObjectLiteralAccessor(literal, it->second->getter, getter);
     VisitObjectLiteralAccessor(literal, it->second->setter, setter);
     builder()
         ->LoadLiteral(Smi::FromInt(NONE))
         .StoreAccumulatorInRegister(attr)
         .CallRuntime(Runtime::kDefineAccessorPropertyUnchecked, literal, 5);
   }
 
   // Object literals have two parts. The "static" part on the left contains no
   // computed property names, and so we can compute its map ahead of time; see
   // Runtime_CreateObjectLiteralBoilerplate. The second "dynamic" part starts
   // with the first computed property name and continues with all properties to
   // its right. All the code from above initializes the static component of the
   // object literal, and arranges for the map of the result to reflect the
   // static order in which the keys appear. For the dynamic properties, we
   // compile them into a series of "SetOwnProperty" runtime calls. This will
   // preserve insertion order.
   for (; property_index < expr->properties()->length(); property_index++) {
     ObjectLiteral::Property* property = expr->properties()->at(property_index);
 
     if (literal_in_accumulator) {
-      literal = temporary_register_scope.NewRegister();
+      temporary_register_scope.PrepareForConsecutiveAllocations(4);
+      literal = temporary_register_scope.NextConsecutiveRegister();
       builder()->StoreAccumulatorInRegister(literal);
       literal_in_accumulator = false;
     }
 
     if (property->kind() == ObjectLiteral::Property::PROTOTYPE) {
       DCHECK(property->emit_store());
       TemporaryRegisterScope inner_temporary_register_scope(builder());
       Register value = inner_temporary_register_scope.NewRegister();
       DCHECK(Register::AreContiguous(literal, value));
-      Visit(property->value());
+      VisitForAccumulatorValue(property->value());
       builder()->StoreAccumulatorInRegister(value).CallRuntime(
           Runtime::kInternalSetPrototype, literal, 2);
       continue;
     }
 
     TemporaryRegisterScope inner_temporary_register_scope(builder());
-    Register key = inner_temporary_register_scope.NewRegister();
-    Register value = inner_temporary_register_scope.NewRegister();
-    Register attr = inner_temporary_register_scope.NewRegister();
+    inner_temporary_register_scope.PrepareForConsecutiveAllocations(3);
+    Register key = inner_temporary_register_scope.NextConsecutiveRegister();
+    Register value = inner_temporary_register_scope.NextConsecutiveRegister();
+    Register attr = inner_temporary_register_scope.NextConsecutiveRegister();
     DCHECK(Register::AreContiguous(literal, key, value, attr));
 
-    Visit(property->key());
+    VisitForAccumulatorValue(property->key());
     builder()->CastAccumulatorToName().StoreAccumulatorInRegister(key);
-    Visit(property->value());
+    VisitForAccumulatorValue(property->value());
     builder()->StoreAccumulatorInRegister(value);
     VisitSetHomeObject(value, literal, property);
     builder()->LoadLiteral(Smi::FromInt(NONE)).StoreAccumulatorInRegister(attr);
     Runtime::FunctionId function_id = static_cast<Runtime::FunctionId>(-1);
     switch (property->kind()) {
       case ObjectLiteral::Property::CONSTANT:
       case ObjectLiteral::Property::COMPUTED:
       case ObjectLiteral::Property::MATERIALIZED_LITERAL:
         function_id = Runtime::kDefineDataPropertyUnchecked;
         break;
@@ skipping 13 matching lines @@
   // Transform literals that contain functions to fast properties.
   if (expr->has_function()) {
     DCHECK(!literal_in_accumulator);
     builder()->CallRuntime(Runtime::kToFastProperties, literal, 1);
   }
 
   if (!literal_in_accumulator) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
   // Deep-copy the literal boilerplate.
   builder()
       ->LoadLiteral(expr->constant_elements())
       .CreateArrayLiteral(expr->literal_index(), expr->ComputeFlags(true));
 
   TemporaryRegisterScope temporary_register_scope(builder());
@@ skipping 11 matching lines @@
       UNIMPLEMENTED();
     }
 
     if (literal_in_accumulator) {
       index = temporary_register_scope.NewRegister();
       literal = temporary_register_scope.NewRegister();
       builder()->StoreAccumulatorInRegister(literal);
       literal_in_accumulator = false;
     }
 
+    FeedbackVectorSlot slot = expr->LiteralFeedbackSlot();
     builder()
         ->LoadLiteral(Smi::FromInt(array_index))
         .StoreAccumulatorInRegister(index);
-    Visit(subexpr);
-    FeedbackVectorSlot slot = expr->LiteralFeedbackSlot();
+    VisitForAccumulatorValue(subexpr);
     builder()->StoreKeyedProperty(literal, index, feedback_index(slot),
                                   language_mode());
   }
 
   if (!literal_in_accumulator) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitVariableProxy(VariableProxy* proxy) {
   VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
 }
 
 
 void BytecodeGenerator::VisitVariableLoad(Variable* variable,
                                           FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
-      Register source(variable->index());
-      builder()->LoadAccumulatorWithRegister(source);
-      // TODO(rmcilroy): Perform check for uninitialized legacy const, const and
-      // let variables.
+      Register source(Register(variable->index()));
+      result_scope()->SetResultInRegister(source);
       break;
     }
     case VariableLocation::PARAMETER: {
       // The parameter indices are shifted by 1 (receiver is variable
       // index -1 but is parameter index 0 in BytecodeArrayBuilder).
-      Register source(builder()->Parameter(variable->index() + 1));
-      builder()->LoadAccumulatorWithRegister(source);
+      Register source = builder()->Parameter(variable->index() + 1);
+      result_scope()->SetResultInRegister(source);
       break;
     }
     case VariableLocation::GLOBAL: {
       // Global var, const, or let variable.
       // TODO(rmcilroy): If context chain depth is short enough, do this using
       // a generic version of LoadGlobalViaContextStub rather than calling the
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->LoadGlobal(variable->index());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::UNALLOCATED: {
       TemporaryRegisterScope temporary_register_scope(builder());
       Register obj = temporary_register_scope.NewRegister();
       builder()->LoadContextSlot(execution_context()->reg(),
                                  Context::GLOBAL_OBJECT_INDEX);
       builder()->StoreAccumulatorInRegister(obj);
       builder()->LoadLiteral(variable->name());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::CONTEXT: {
       ContextScope* context = execution_context()->Previous(variable->scope());
       if (context) {
         builder()->LoadContextSlot(context->reg(), variable->index());
       } else {
         UNIMPLEMENTED();
       }
+      result_scope()->SetResultInAccumulator();
       // TODO(rmcilroy): Perform check for uninitialized legacy const, const and
       // let variables.
       break;
     }
     case VariableLocation::LOOKUP:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
                                                 FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
       // TODO(rmcilroy): support const mode initialization.
       Register destination(variable->index());
       builder()->StoreAccumulatorInRegister(destination);
+      RecordStoreToRegister(destination);
       break;
     }
     case VariableLocation::PARAMETER: {
       // The parameter indices are shifted by 1 (receiver is variable
       // index -1 but is parameter index 0 in BytecodeArrayBuilder).
       Register destination(builder()->Parameter(variable->index() + 1));
       builder()->StoreAccumulatorInRegister(destination);
+      RecordStoreToRegister(destination);
       break;
     }
     case VariableLocation::GLOBAL: {
       // Global var, const, or let variable.
       // TODO(rmcilroy): If context chain depth is short enough, do this using
       // a generic version of LoadGlobalViaContextStub rather than calling the
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->StoreGlobal(variable->index(), language_mode());
       break;
     }
     case VariableLocation::UNALLOCATED: {
-      TemporaryRegisterScope temporary_register_scope(builder());
-      Register value = temporary_register_scope.NewRegister();
-      Register obj = temporary_register_scope.NewRegister();
-      Register name = temporary_register_scope.NewRegister();
+      Register value = result_scope()->NewRegister();
+      Register obj = result_scope()->NewRegister();
+      Register name = result_scope()->NewRegister();
+
       // TODO(rmcilroy): Investigate whether we can avoid having to stash the
       // value in a register.
       builder()->StoreAccumulatorInRegister(value);
       builder()->LoadContextSlot(execution_context()->reg(),
                                  Context::GLOBAL_OBJECT_INDEX);
       builder()->StoreAccumulatorInRegister(obj);
       builder()->LoadLiteral(variable->name());
       builder()->StoreAccumulatorInRegister(name);
       builder()->LoadAccumulatorWithRegister(value);
       builder()->StoreNamedProperty(obj, name, feedback_index(slot),
@@ skipping 11 matching lines @@
       break;
     }
     case VariableLocation::LOOKUP:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   DCHECK(expr->target()->IsValidReferenceExpression());
-  TemporaryRegisterScope temporary_register_scope(builder());
   Register object, key;
 
   // Left-hand side can only be a property, a global or a variable slot.
   Property* property = expr->target()->AsProperty();
   LhsKind assign_type = Property::GetAssignType(property);
 
   // Evaluate LHS expression.
   switch (assign_type) {
     case VARIABLE:
       // Nothing to do to evaluate variable assignment LHS.
       break;
-    case NAMED_PROPERTY:
-      object = temporary_register_scope.NewRegister();
-      key = temporary_register_scope.NewRegister();
-      Visit(property->obj());
-      builder()->StoreAccumulatorInRegister(object);
+    case NAMED_PROPERTY: {
+      object = VisitForRegisterValue(property->obj());
+      key = result_scope()->NewRegister();
       builder()->LoadLiteral(property->key()->AsLiteral()->AsPropertyName());
       builder()->StoreAccumulatorInRegister(key);
       break;
-    case KEYED_PROPERTY:
-      object = temporary_register_scope.NewRegister();
-      key = temporary_register_scope.NewRegister();
-      Visit(property->obj());
-      builder()->StoreAccumulatorInRegister(object);
-      Visit(property->key());
-      builder()->StoreAccumulatorInRegister(key);
+    }
+    case KEYED_PROPERTY: {
+      object = VisitForRegisterValue(property->obj());
+      key = VisitForRegisterValue(property->key());
       break;
+    }
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
       UNIMPLEMENTED();
   }
 
   // Evaluate the value and potentially handle compound assignments by loading
   // the left-hand side value and performing a binary operation.
   if (expr->is_compound()) {
     UNIMPLEMENTED();
   } else {
-    Visit(expr->value());
+    VisitForAccumulatorValue(expr->value());
+    result_scope()->SetResultInAccumulator();
   }
 
   // Store the value.
   FeedbackVectorSlot slot = expr->AssignmentSlot();
   switch (assign_type) {
     case VARIABLE: {
       Variable* variable = expr->target()->AsVariableProxy()->var();
       VisitVariableAssignment(variable, slot);
       break;
     }
@@ skipping 20 matching lines @@
 
 void BytecodeGenerator::VisitPropertyLoad(Register obj, Property* expr) {
   LhsKind property_kind = Property::GetAssignType(expr);
   FeedbackVectorSlot slot = expr->PropertyFeedbackSlot();
   switch (property_kind) {
     case VARIABLE:
       UNREACHABLE();
     case NAMED_PROPERTY: {
       builder()->LoadLiteral(expr->key()->AsLiteral()->AsPropertyName());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case KEYED_PROPERTY: {
-      Visit(expr->key());
+      VisitForAccumulatorValue(expr->key());
       builder()->LoadKeyedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitProperty(Property* expr) {
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register obj = temporary_register_scope.NewRegister();
-  Visit(expr->obj());
-  builder()->StoreAccumulatorInRegister(obj);
+  Register obj = VisitForRegisterValue(expr->obj());
   VisitPropertyLoad(obj, expr);
 }
 
 
-Register BytecodeGenerator::VisitArguments(
-    ZoneList<Expression*>* args, TemporaryRegisterScope* register_scope) {
+Register BytecodeGenerator::VisitArguments(ZoneList<Expression*>* args) {
   // Visit arguments and place in a contiguous block of temporary registers.
   // Return the first temporary register corresponding to the first argument.
-  DCHECK_GT(args->length(), 0);
-  Register first_arg = register_scope->NewRegister();
-  Visit(args->at(0));
+  result_scope()->PrepareForConsecutiveAllocations(args->length());
+
+  // Visit for first argument that goes into returned register
+  Register first_arg = result_scope()->NextConsecutiveRegister();
+  VisitForAccumulatorValue(args->at(0));
   builder()->StoreAccumulatorInRegister(first_arg);
+
+  // Visit remaining arguments
   for (int i = 1; i < static_cast<int>(args->length()); i++) {
-    Register ith_arg = register_scope->NewRegister();
-    Visit(args->at(i));
+    Register ith_arg = result_scope()->NextConsecutiveRegister();
+    VisitForAccumulatorValue(args->at(i));
     builder()->StoreAccumulatorInRegister(ith_arg);
     DCHECK(ith_arg.index() - i == first_arg.index());
   }
-
   return first_arg;
 }
 
 
 void BytecodeGenerator::VisitCall(Call* expr) {
   Expression* callee_expr = expr->expression();
   Call::CallType call_type = expr->GetCallType(isolate());
 
   // Prepare the callee and the receiver to the function call. This depends on
   // the semantics of the underlying call type.
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register callee = temporary_register_scope.NewRegister();
-  Register receiver = temporary_register_scope.NewRegister();
+  Register callee = result_scope()->NewRegister();
+
+  // The receiver and arguments need to be allocated consecutively for
+  // Call(). Future optimizations could avoid this there are no
+  // arguments or the receiver and arguments are already consecutive.
+  ZoneList<Expression*>* args = expr->arguments();
+  result_scope()->PrepareForConsecutiveAllocations(args->length() + 1);

[rmcilroy, 2015/10/20 16:39:20]

You call PrepareForConsecutiveAllocations(args->length() + 1) and
>PrepareForConsecutiveAllocations(args->length()) in VisitArguments below. Not
sure if this is a problem, but it is probably worth a comment

[oth, 2015/10/20 20:58:42]

Comment added to VisitArguments.

The weirdness is introduced because we don't pass first_arg as an argument in
the Call operation, but just pass the receiver.

There is a simple statemachine inside the TemporaryRegisterScope that keeps
track of the next register to
be allocated in the consecutive sequence and the number remaining in the
consecutive sequence. A call to PrepareForConsecutiveAllocations has no effect
if the number requested is <= the existing number of remaining consecutives
allocations. If the request is larger then there's the searching/allocating
temporary registers to be consecutive in the BytecodeArrayBuilder.

e.g. PFCA(5); NewConsecutiveRegister(); PFCA(4) -> second call is no-op
PFCA(4); NewConsecutiveRegister(); PFCA(4) -> second call completely resets
internal state.

So long as all the allocations are properly scoped then this works fine. We
DCHECK if scoping is deemed wrong because consecutive allocations can not be
fulfilled.

Having pushed on this, I've found the code is brittle with respect to errors
here (which is good).

+  Register receiver = result_scope()->NextConsecutiveRegister();
 
   switch (call_type) {
     case Call::PROPERTY_CALL: {
       Property* property = callee_expr->AsProperty();
       if (property->IsSuperAccess()) {
         UNIMPLEMENTED();
       }
-      Visit(property->obj());
+      VisitForAccumulatorValue(property->obj());
       builder()->StoreAccumulatorInRegister(receiver);
+      // Need a result scope here to keep our consecutive
+      // temporaries.
+      AccumulatorResultScope accumulator_result_scope(this);
       // Perform a property load of the callee.
       VisitPropertyLoad(receiver, property);
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::GLOBAL_CALL: {
+      AccumulatorResultScope accumulator_result_scope(this);

[rmcilroy, 2015/10/20 16:39:20]

Not sure why this is needed? Could you add a comment explaining it if it is?

[oth, 2015/10/20 20:58:42]

Comment added and scope moved immediately before VisitVariableLoad. Essentially
we don't want VisitVariableLoad using and not freeing our temporaries nor
setting our result (it's that calling SetResultInAccumulator twice issue).

       // Receiver is undefined for global calls.
       builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
       // Load callee as a global variable.
       VariableProxy* proxy = callee_expr->AsVariableProxy();
       VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::OTHER_CALL: {
       builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
-      Visit(callee_expr);
+      VisitForAccumulatorValue(callee_expr);
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::LOOKUP_SLOT_CALL:
     case Call::SUPER_CALL:
     case Call::POSSIBLY_EVAL_CALL:
       UNIMPLEMENTED();
   }
 
   // Evaluate all arguments to the function call and store in sequential
   // registers.
-  ZoneList<Expression*>* args = expr->arguments();
   if (args->length() > 0) {
-    Register first_arg = VisitArguments(args, &temporary_register_scope);
-    CHECK_EQ(first_arg.index(), receiver.index() + 1);
+    Register arg = VisitArguments(args);
+    CHECK(arg.index() == receiver.index() + 1);
   }
 
   // TODO(rmcilroy): Deal with possible direct eval here?
   // TODO(rmcilroy): Use CallIC to allow call type feedback.
   builder()->Call(callee, receiver, args->length());
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitCallNew(CallNew* expr) {
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register constructor = temporary_register_scope.NewRegister();
-  Visit(expr->expression());
+  Register constructor = result_scope()->NewRegister();
+  VisitForAccumulatorValue(expr->expression());
   builder()->StoreAccumulatorInRegister(constructor);
+
   ZoneList<Expression*>* args = expr->arguments();
   if (args->length() > 0) {
-    Register first_arg = VisitArguments(args, &temporary_register_scope);
+    Register first_arg = VisitArguments(args);
     builder()->New(constructor, first_arg, args->length());
   } else {
     // The second argument here will be ignored as there are zero
     // arguments. Using the constructor register avoids avoid
     // allocating a temporary just to fill the operands.
     builder()->New(constructor, constructor, 0);
   }
+
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
   if (expr->is_jsruntime()) {
     UNIMPLEMENTED();
   }
 
-  // Evaluate all arguments to the runtime call.
-  TemporaryRegisterScope temporary_register_scope(&builder_);
-
   // TODO(rmcilroy): support multiple return values.
   DCHECK_LE(expr->function()->result_size, 1);
   Runtime::FunctionId function_id = expr->function()->function_id;
+
+  // Evaluate all arguments to the runtime call.
   ZoneList<Expression*>* args = expr->arguments();
   Register first_arg;
   if (args->length() > 0) {
-    first_arg = VisitArguments(args, &temporary_register_scope);
+    first_arg = VisitArguments(args);
   } else {
     // Allocation here is just to fullfil the requirement that there
     // is a register operand for the start of the arguments though
     // there are zero when this is generated.
-    first_arg = temporary_register_scope.NewRegister();
+    first_arg = result_scope()->NewRegister();
   }
   builder()->CallRuntime(function_id, first_arg, args->length());
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitVoid(UnaryOperation* expr) {
-  Visit(expr->expression());
+  VisitForEffect(expr->expression());
   builder()->LoadUndefined();
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitTypeOf(UnaryOperation* expr) {
-  Visit(expr->expression());
+  VisitForAccumulatorValue(expr->expression());
   builder()->TypeOf();
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitNot(UnaryOperation* expr) {
-  Visit(expr->expression());
+  VisitForAccumulatorValue(expr->expression());
   builder()->LogicalNot();
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
   switch (expr->op()) {
     case Token::Value::NOT:
       VisitNot(expr);
       break;
     case Token::Value::TYPEOF:
       VisitTypeOf(expr);
@@ skipping 27 matching lines @@
       VisitLogicalAndExpression(binop);
       break;
     default:
       VisitArithmeticExpression(binop);
       break;
   }
 }
 
 
 void BytecodeGenerator::VisitCompareOperation(CompareOperation* expr) {
-  Token::Value op = expr->op();
-  Expression* left = expr->left();
-  Expression* right = expr->right();
-
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register temporary = temporary_register_scope.NewRegister();
-
-  Visit(left);
-  builder()->StoreAccumulatorInRegister(temporary);
-  Visit(right);
-  builder()->CompareOperation(op, temporary, language_mode_strength());
+  // TODO(oth): add binary operation scope here to replace

[rmcilroy, 2015/10/20 16:39:20]

As mentioned in the previous review, I don't think this should be a binary
operation scope at all, but just a StatementScope. AFAICT you only use the
binary_expression_depth_ to work out when to clear the
binary_expression_hazard_set_ - if this is the case then if you hold the
hazard_set_ in the StatementScope you don't need to track binary expression
depth, just clear it when the StatementScope goes out of scope. This might
extend the hazard lifetime slightly longer than needed  but shouldn't affect
normal code and I think it's simpler and less error prone. If you agree,
updating the TODO for now is fine, otherwise let's discuss this in-person
tomorrow.

[oth, 2015/10/20 20:58:42]

Done - grumble, grumble :-) We can go conservative here. We know we'll revisit
if we find need anyway.

+  // {Prepare|Complete}BinaryExpression. This will maintain a counter in the
+  // statement or top-level expression scope.
+  PrepareForBinaryExpression();
+  Register lhs = VisitForRegisterValue(expr->left());
+  if (builder()->RegisterIsParameterOrLocal(lhs)) {
+    // Result was returned in an existing local or parameter. See if
+    // it needs to be moved to a temporary.
+    // TODO(oth) LoadFromAliasedRegister call into VisitVariableLoad().
+    lhs = LoadFromAliasedRegister(lhs);
+  }
+  VisitForAccumulatorValue(expr->right());
+  builder()->CompareOperation(expr->op(), lhs, language_mode_strength());
+  result_scope()->SetResultInAccumulator();
+  CompleteBinaryExpression();
 }
 
 
+void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* expr) {
+  // TODO(oth): add binary operation scope here to replace
+  // {Prepare|Complete}BinaryExpression. This will maintain a counter in the
+  // statement or top-level expression scope.

[rmcilroy, 2015/10/20 16:39:20]

ditto

[oth, 2015/10/20 20:58:42]

Done.

+  PrepareForBinaryExpression();
+  Register lhs = VisitForRegisterValue(expr->left());
+  if (builder()->RegisterIsParameterOrLocal(lhs)) {
+    // Result was returned in an existing local or parameter. See if
+    // it needs to be moved to a temporary.
+    // TODO(oth) LoadFromAliasedRegister call into VisitVariableLoad().
+    lhs = LoadFromAliasedRegister(lhs);
+  }
+  VisitForAccumulatorValue(expr->right());
+  builder()->BinaryOperation(expr->op(), lhs, language_mode_strength());
+  result_scope()->SetResultInAccumulator();
+  CompleteBinaryExpression();
+}
+
+
 void BytecodeGenerator::VisitSpread(Spread* expr) { UNREACHABLE(); }
 
 
 void BytecodeGenerator::VisitEmptyParentheses(EmptyParentheses* expr) {
   UNREACHABLE();
 }
 
 
 void BytecodeGenerator::VisitThisFunction(ThisFunction* expr) {
   UNIMPLEMENTED();
@@ skipping 68 matching lines @@
   builder()
       ->LoadLiteral(scope->GetScopeInfo(isolate()))
       .StoreAccumulatorInRegister(scope_info);
   VisitFunctionClosureForContext();
   builder()
       ->StoreAccumulatorInRegister(closure)
       .CallRuntime(Runtime::kPushBlockContext, scope_info, 2);
 }
 
 
-void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* binop) {
-  Token::Value op = binop->op();
-  Expression* left = binop->left();
-  Expression* right = binop->right();
-
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register temporary = temporary_register_scope.NewRegister();
-
-  Visit(left);
-  builder()->StoreAccumulatorInRegister(temporary);
-  Visit(right);
-  builder()->BinaryOperation(op, temporary, language_mode_strength());
-}
-
-
 void BytecodeGenerator::VisitCommaExpression(BinaryOperation* binop) {
-  Expression* left = binop->left();
-  Expression* right = binop->right();
-
-  Visit(left);
-  Visit(right);
+  VisitForEffect(binop->left());
+  Visit(binop->right());
 }
 
 
 void BytecodeGenerator::VisitLogicalOrExpression(BinaryOperation* binop) {
   Expression* left = binop->left();
   Expression* right = binop->right();
 
   // Short-circuit evaluation- If it is known that left is always true,
   // no need to visit right
   if (left->ToBooleanIsTrue()) {
-    Visit(left);
+    VisitForAccumulatorValue(left);
   } else {
     BytecodeLabel end_label;
-
-    Visit(left);
+    VisitForAccumulatorValue(left);
     builder()->JumpIfToBooleanTrue(&end_label);
-    Visit(right);
+    VisitForAccumulatorValue(right);
     builder()->Bind(&end_label);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitLogicalAndExpression(BinaryOperation* binop) {
   Expression* left = binop->left();
   Expression* right = binop->right();
 
   // Short-circuit evaluation- If it is known that left is always false,
   // no need to visit right
   if (left->ToBooleanIsFalse()) {
-    Visit(left);
+    VisitForAccumulatorValue(left);
   } else {
     BytecodeLabel end_label;
-
-    Visit(left);
+    VisitForAccumulatorValue(left);
     builder()->JumpIfToBooleanFalse(&end_label);
-    Visit(right);
+    VisitForAccumulatorValue(right);
     builder()->Bind(&end_label);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitObjectLiteralAccessor(
     Register home_object, ObjectLiteralProperty* property, Register value_out) {
   // TODO(rmcilroy): Replace value_out with VisitForRegister();
   if (property == nullptr) {
     builder()->LoadNull().StoreAccumulatorInRegister(value_out);
   } else {
-    Visit(property->value());
+    VisitForAccumulatorValue(property->value());
     builder()->StoreAccumulatorInRegister(value_out);
     VisitSetHomeObject(value_out, home_object, property);
   }
 }
 
 
 void BytecodeGenerator::VisitSetHomeObject(Register value, Register home_object,
                                            ObjectLiteralProperty* property,
                                            int slot_number) {
   Expression* expr = property->value();
@@ skipping 23 matching lines @@
     // their closure, not the anonymous closure containing the global code.
     // Pass a SMI sentinel and let the runtime look up the empty function.
     builder()->LoadLiteral(Smi::FromInt(0));
   } else {
     DCHECK(closure_scope->is_function_scope());
     builder()->LoadAccumulatorWithRegister(Register::function_closure());
   }
 }
 
 
+void BytecodeGenerator::PrepareForBinaryExpression() {
+  if (binary_expression_depth_++ == 0) {
+    binary_expression_hazard_set_.clear();
+  }
+}
+
+
+// Visits the expression |expr| and places the result in the accumulator.
+void BytecodeGenerator::VisitForAccumulatorValue(Expression* expr) {
+  AccumulatorResultScope accumulator_scope(this);
+  Visit(expr);
+}
+
+
+// Visits the expression |expr| and discards the result.
+void BytecodeGenerator::VisitForEffect(Expression* expr) {
+  EffectResultScope effect_scope(this);
+  Visit(expr);
+}
+
+
+// Visits the expression |expr| and places the result in |temporary_register|
+// if the result is not already a register value. The return value is
+// the register containing the result. The caller should check whether
+// this is the temporary register and consider releasing the temporary
+// register if not.

[rmcilroy, 2015/10/20 16:39:20]

Comment now out of date (no need to release temporary variable)

[oth, 2015/10/20 20:58:42]

Done.

+Register BytecodeGenerator::VisitForRegisterValue(Expression* expr) {
+  RegisterResultScope register_scope(this);
+  Visit(expr);
+  return register_scope.ResultRegister();
+}
+
+
+Register BytecodeGenerator::LoadFromAliasedRegister(Register reg) {
+  // TODO(oth): Follow on CL to load from re-map here.
+  DCHECK(builder()->RegisterIsParameterOrLocal(reg));
+  if (binary_expression_depth_ > 0) {
+    binary_expression_hazard_set_.insert(reg.index());
+  }
+  return reg;
+}
+
+
+void BytecodeGenerator::RecordStoreToRegister(Register reg) {
+  DCHECK(builder()->RegisterIsParameterOrLocal(reg));
+  if (binary_expression_depth_ > 0) {
+    // TODO(oth): a store to a register that's be loaded needs to be
+    // remapped.
+    DCHECK(binary_expression_hazard_set_.find(reg.index()) ==
+           binary_expression_hazard_set_.end());
+  }
+}
+
+
+void BytecodeGenerator::CompleteBinaryExpression() {
+  DCHECK(binary_expression_depth_ > 0);
+  binary_expression_depth_ -= 1;
+  // TODO(oth): spill remapped registers into origins.
+  // TODO(oth): make statement/top-level.
+}
+
+
 Register BytecodeGenerator::NextContextRegister() const {
   if (execution_context() == nullptr) {
     // Return the incoming function context for the outermost execution context.
     return Register::function_context();
   }
   Register previous = execution_context()->reg();
   if (previous == Register::function_context()) {
     // If the previous context was the incoming function context, then the next
     // context register is the first local context register.
     return builder_.first_context_register();
@@ skipping 15 matching lines @@
 }
 
 
 int BytecodeGenerator::feedback_index(FeedbackVectorSlot slot) const {
   return info()->feedback_vector()->GetIndex(slot);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8