[Interpreter] Reduce temporary register usage in generated bytecode.

src/interpreter/bytecode-generator.cc

Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc
index 309b9294297674b1b6ff75e4214df11865624620..5cf1286545db5cba812d02a8c4ab5d773c26cfd0 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -4,8 +4,6 @@
 
 #include "src/interpreter/bytecode-generator.h"
 
-#include <stack>
-
 #include "src/compiler.h"
 #include "src/interpreter/control-flow-builders.h"
 #include "src/objects.h"
@@ -146,6 +144,132 @@ void BytecodeGenerator::ControlScope::PerformCommand(Command command,
 }
 
 
+// Scoped base class for determining where the result of an expression
+// is stored.
+class BytecodeGenerator::ExpressionResultScope {
+ public:
+  explicit ExpressionResultScope(BytecodeGenerator* generator)
+      : generator_(generator),
+        parent_(generator->result_scope()),
+        allocator_(builder()),
+        result_identified_(false) {
+    generator_->set_result_scope(this);
+  }
+
+  virtual ~ExpressionResultScope() {
+    generator_->set_result_scope(parent_);
+    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* parent() const { return parent_; }

[rmcilroy, 2015/10/19 12:56:22]

nit - outer() and outer_ to be consistent with other scopes?

[oth, 2015/10/20 15:28:52]

Done.

+
+  Register NewRegister() { return allocator_.NewRegister(); }
+  void PrepareForConsecutiveAllocations(size_t count) {
+    allocator_.PrepareForConsecutiveAllocations(count);
+  }
+
+ protected:
+  void set_result_identified() {
+    DCHECK(!result_identified());
+    result_identified_ = true;
+  }
+
+  bool result_identified() const { return result_identified_; }
+
+ private:
+  BytecodeGenerator* generator_;
+  ExpressionResultScope* parent_;
+  TemporaryRegisterScope allocator_;
+  bool result_identified_;
+  DISALLOW_COPY_AND_ASSIGN(ExpressionResultScope);
+};
+
+
+// Scoped class used for the top-level management of an expression scope.
+// Specifically mapping registers across an expresion.

[rmcilroy, 2015/10/19 12:56:22]

expression / expresion (although this class should just be removed for now
unless we start using it in this CL).

[oth, 2015/10/20 15:28:52]

Done.

+class BytecodeGenerator::TopLevelResultScope final
+    : public ExpressionResultScope {
+ public:
+  explicit TopLevelResultScope(BytecodeGenerator* generator)
+      : ExpressionResultScope(generator) {
+    generator->set_top_level_result_scope(this);
+    set_result_identified();
+  }
+  virtual ~TopLevelResultScope() {

[rmcilroy, 2015/10/19 12:56:22]

nit newline above

[oth, 2015/10/20 15:28:52]

Class removed.

+    generator()->set_top_level_result_scope(nullptr);
+  }
+
+  virtual void SetResultInAccumulator() { set_result_identified(); }
+  virtual void SetResultInRegister(Register reg) { set_result_identified(); }
+};
+
+
+// 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() {
+    if (!result_identified()) {
+      // TODO(oth): REVIEW.

[rmcilroy, 2015/10/19 12:56:22]

Can we remove the "if (!result_identified())" now?

[oth, 2015/10/20 15:28:52]

Done.

+      set_result_identified();
+    }
+  }
+  virtual void SetResultInRegister(Register reg) {

[rmcilroy, 2015/10/19 12:56:23]

nit - newline above

[oth, 2015/10/20 15:28:52]

Done.

+    builder()->LoadAccumulatorWithRegister(reg);
+    set_result_identified();
+  }
+};
+
+
+// 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) {}
+};

[rmcilroy, 2015/10/19 12:56:22]

nit - could you move this above BytecodeGenerator::AccumulatorResultScope

[oth, 2015/10/20 15:28:52]

Done.

+
+
+// 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_ = parent()->NewRegister();
+    builder()->StoreAccumulatorInRegister(result_register_);
+    set_result_identified();
+  }
+
+  virtual void SetResultInRegister(Register 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),
@@ -154,7 +278,11 @@ BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
       scope_(nullptr),
       globals_(0, zone),
       execution_control_(nullptr),
-      execution_context_(nullptr) {
+      execution_context_(nullptr),
+      result_scope_(nullptr),
+      top_level_result_scope_(nullptr),
+      binary_expression_depth_(0),
+      binary_expression_hazard_set_(zone) {
   InitializeAstVisitor(isolate);
 }
 
@@ -162,6 +290,35 @@ BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
 BytecodeGenerator::~BytecodeGenerator() {}
 
 
+void BytecodeGenerator::set_result_scope(ExpressionResultScope* result_scope) {
+  result_scope_ = result_scope;

[rmcilroy, 2015/10/19 12:56:22]

nit - just inline this in header

[oth, 2015/10/20 15:28:52]

Done.

+}
+
+
+BytecodeGenerator::ExpressionResultScope* BytecodeGenerator::result_scope()

[rmcilroy, 2015/10/19 12:56:23]

ditto

[oth, 2015/10/20 15:28:52]

Done.

+    const {
+  return result_scope_;
+}
+
+
+BytecodeGenerator::ExpressionResultScope*
+BytecodeGenerator::parent_result_scope() const {

[rmcilroy, 2015/10/19 12:56:22]

Unused, please remove (callers could just do result_scope()->parent() anyway if
needed)

[oth, 2015/10/20 15:28:52]

Done.

+  return result_scope_->parent();
+}
+
+void BytecodeGenerator::set_top_level_result_scope(
+    TopLevelResultScope* result_scope) {
+  DCHECK((result_scope == nullptr && top_level_result_scope_ != nullptr) ||
+         (result_scope != nullptr && top_level_result_scope_ == nullptr));
+  top_level_result_scope_ = result_scope;
+}
+
+BytecodeGenerator::TopLevelResultScope*
+BytecodeGenerator::top_level_result_scope() const {
+  return top_level_result_scope_;
+}
+
+
 Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   set_info(info);
   set_scope(info->scope());
@@ -324,12 +481,12 @@ void BytecodeGenerator::VisitDeclarations(
 
 
 void BytecodeGenerator::VisitExpressionStatement(ExpressionStatement* stmt) {
-  Visit(stmt->expression());
+  TopLevelResultScope result_scope(this);
+  VisitForAccumulatorValue(stmt->expression());

[rmcilroy, 2015/10/19 12:56:23]

VisitForEffect here I think? Following statements should never be expecting
anything in the accumulator

[oth, 2015/10/20 15:28:53]

Done.

 }
 
 
 void BytecodeGenerator::VisitEmptyStatement(EmptyStatement* stmt) {
-  // TODO(oth): For control-flow it could be useful to signal empty paths here.
 }
 
 
@@ -340,7 +497,7 @@ void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
 
   BytecodeLabel else_label, end_label;
 
-  Visit(stmt->condition());
+  VisitForAccumulatorValue(stmt->condition());
   builder()->CastAccumulatorToBoolean();
   builder()->JumpIfFalse(&else_label);
   Visit(stmt->then_statement());
@@ -372,7 +529,8 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  TopLevelResultScope scope(this);

[rmcilroy, 2015/10/19 12:56:22]

Given my comment above about removing TopLevelResultScope for now this is not
useful in this CL, however, how about StatementResultScope instead (TopLevel is
a bit overloaded).

[oth, 2015/10/20 15:28:53]

Ack. Removed for now, the newer name is fine.

+  VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
@@ -398,7 +556,7 @@ void BytecodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
   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);
 
@@ -416,7 +574,7 @@ void BytecodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
   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);
 
@@ -445,7 +603,7 @@ void BytecodeGenerator::VisitForStatement(ForStatement* stmt) {
   }
   if (stmt->cond()) {
     builder()->Bind(&condition_label);
-    Visit(stmt->cond());
+    VisitForAccumulatorValue(stmt->cond());
     builder()->JumpIfTrue(&body_label);
   } else {
     builder()->Jump(&body_label);
@@ -500,6 +658,7 @@ void BytecodeGenerator::VisitFunctionLiteral(FunctionLiteral* expr) {
   builder()
       ->LoadLiteral(shared_info)
       .CreateClosure(expr->pretenure() ? TENURED : NOT_TENURED);
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -534,6 +693,7 @@ void BytecodeGenerator::VisitLiteral(Literal* expr) {
   } else {
     builder()->LoadLiteral(value);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -546,6 +706,7 @@ void BytecodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
       .StoreAccumulatorInRegister(flags)
       .LoadLiteral(expr->pattern())
       .CreateRegExpLiteral(expr->literal_index(), flags);
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -590,21 +751,24 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
             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());
+            VisitForAccumulatorValue(property->value());

[rmcilroy, 2015/10/19 12:56:22]

VisitForEffect here

[oth, 2015/10/20 15:28:53]

Done.

           }
         } else {
           Register key = inner_temporary_register_scope.NewRegister();
           Register value = inner_temporary_register_scope.NewRegister();
           Register language = inner_temporary_register_scope.NewRegister();
+          // 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/19 12:56:22]

Can we use PrepareForConsecutiveAllocations at the top of VisitObjectLiteral ?

[oth, 2015/10/20 15:28:53]

Done.

           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()
@@ -620,7 +784,7 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
         DCHECK(property->emit_store());
         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);
         break;
@@ -648,7 +812,7 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
     Register setter = inner_temporary_register_scope.NewRegister();
     Register attr = inner_temporary_register_scope.NewRegister();
     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);
@@ -671,6 +835,7 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
     ObjectLiteral::Property* property = expr->properties()->at(property_index);
 
     if (literal_in_accumulator) {
+      temporary_register_scope.PrepareForConsecutiveAllocations(4);
       literal = temporary_register_scope.NewRegister();
       builder()->StoreAccumulatorInRegister(literal);
       literal_in_accumulator = false;
@@ -681,21 +846,22 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
       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());
+    inner_temporary_register_scope.PrepareForConsecutiveAllocations(3);
     Register key = inner_temporary_register_scope.NewRegister();
     Register value = inner_temporary_register_scope.NewRegister();
     Register attr = inner_temporary_register_scope.NewRegister();
     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);
@@ -729,6 +895,7 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -763,7 +930,7 @@ void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
     builder()
         ->LoadLiteral(Smi::FromInt(array_index))
         .StoreAccumulatorInRegister(index);
-    Visit(subexpr);
+    VisitForAccumulatorValue(subexpr);
     FeedbackVectorSlot slot = expr->LiteralFeedbackSlot();

[rmcilroy, 2015/10/19 12:56:22]

nit - (I know this was my code..) could you move the FeedbackSlot up about
builder() so the use of the accumulator is more obvious.

[oth, 2015/10/20 15:28:52]

Done.

     builder()->StoreKeyedProperty(literal, index, feedback_index(slot),
                                   language_mode());
@@ -773,6 +940,7 @@ void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -785,17 +953,15 @@ 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: {
@@ -805,6 +971,7 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->LoadGlobal(variable->index());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::UNALLOCATED: {
@@ -815,6 +982,7 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       builder()->StoreAccumulatorInRegister(obj);
       builder()->LoadLiteral(variable->name());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::CONTEXT: {
@@ -824,6 +992,7 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       } else {
         UNIMPLEMENTED();
       }
+      result_scope()->SetResultInAccumulator();
       // TODO(rmcilroy): Perform check for uninitialized legacy const, const and
       // let variables.
       break;
@@ -841,6 +1010,7 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
       // TODO(rmcilroy): support const mode initialization.
       Register destination(variable->index());
       builder()->StoreAccumulatorInRegister(destination);
+      StoreForBinaryExpression(destination);

[rmcilroy, 2015/10/19 12:56:22]

I don't like this being called StoreForBinaryExpression given that this is
VisitVariableAssignment which doesn't involve binary expressions. How about
RecordStoreToLocalRegister or similar?

[oth, 2015/10/20 15:28:52]

RecordStoreToRegister()? It'll certainly cover parameters and probably
temporaries that mappings of earlier mappings.

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

Good point, SGTM

       break;
     }
     case VariableLocation::PARAMETER: {
@@ -848,6 +1018,7 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
       // index -1 but is parameter index 0 in BytecodeArrayBuilder).
       Register destination(builder()->Parameter(variable->index() + 1));
       builder()->StoreAccumulatorInRegister(destination);
+      StoreForBinaryExpression(destination);
       break;
     }
     case VariableLocation::GLOBAL: {
@@ -860,10 +1031,10 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
       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);
@@ -895,7 +1066,6 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
 
 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.
@@ -907,22 +1077,18 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
     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();
@@ -933,7 +1099,8 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   if (expr->is_compound()) {
     UNIMPLEMENTED();
   } else {
-    Visit(expr->value());
+    VisitForAccumulatorValue(expr->value());
+    result_scope()->SetResultInAccumulator();
   }
 
   // Store the value.
@@ -974,11 +1141,13 @@ void BytecodeGenerator::VisitPropertyLoad(Register obj, Property* expr) {
     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:
@@ -989,25 +1158,21 @@ void BytecodeGenerator::VisitPropertyLoad(Register obj, Property* expr) {
 
 
 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));
+  Register first_arg = result_scope()->NewRegister();
+  VisitForAccumulatorValue(args->at(0));
   builder()->StoreAccumulatorInRegister(first_arg);
   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()->NewRegister();
+    VisitForAccumulatorValue(args->at(i));
     builder()->StoreAccumulatorInRegister(ith_arg);
     DCHECK(ith_arg.index() - i == first_arg.index());
   }
@@ -1017,14 +1182,21 @@ Register BytecodeGenerator::VisitArguments(
 
 
 void BytecodeGenerator::VisitCall(Call* expr) {
+  static int hits = 0;
+  hits++;

[rmcilroy, 2015/10/19 12:56:23]

leftover debugging code?

[oth, 2015/10/20 15:28:53]

Done.

   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);
+  Register receiver = result_scope()->NewRegister();
 
   switch (call_type) {
     case Call::PROPERTY_CALL: {
@@ -1032,8 +1204,11 @@ void BytecodeGenerator::VisitCall(Call* expr) {
       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);
@@ -1050,7 +1225,7 @@ void BytecodeGenerator::VisitCall(Call* expr) {
     }
     case Call::OTHER_CALL: {
       builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
-      Visit(callee_expr);
+      VisitForAccumulatorValue(callee_expr);
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
@@ -1062,26 +1237,30 @@ void BytecodeGenerator::VisitCall(Call* expr) {
 
   // 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);
+  result_scope()->PrepareForConsecutiveAllocations(args->length());

[rmcilroy, 2015/10/19 12:56:22]

nit - could we move PrepareForConsecutiveAllocations into VisitArguments. Also,
shouldn't VisitCall also be using VisitArguments?

[oth, 2015/10/20 15:28:52]

Done.

[oth, 2015/10/20 15:28:52]

Done.

+  for (int i = 0; i < args->length(); ++i) {
+    VisitForAccumulatorValue(args->at(i));
+    Register arg = result_scope()->NewRegister();
+    DCHECK(arg.index() - i == receiver.index() + 1);

[rmcilroy, 2015/10/19 12:56:22]

nit - move Register arg and DCHECK lines above VisitForAccumulator to make use
of accumulator more obvious

[oth, 2015/10/20 15:28:52]

Done.

+    builder()->StoreAccumulatorInRegister(arg);
   }
 
   // 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);
+    result_scope()->PrepareForConsecutiveAllocations(args->length());
+    Register first_arg = VisitArguments(args);
     builder()->New(constructor, first_arg, args->length());
   } else {
     // The second argument here will be ignored as there are zero
@@ -1089,6 +1268,8 @@ void BytecodeGenerator::VisitCallNew(CallNew* expr) {
     // allocating a temporary just to fill the operands.
     builder()->New(constructor, constructor, 0);
   }
+
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -1098,40 +1279,43 @@ void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
   }
 
   // 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;
   ZoneList<Expression*>* args = expr->arguments();
+  result_scope()->PrepareForConsecutiveAllocations(args->length());
   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());
+  // TODO(rmcilroy): support multiple return values.

[rmcilroy, 2015/10/19 12:56:22]

nit - move TODO back up to DCHECK_LE (that is what would fail if we tried to
call something with multiple results.

[oth, 2015/10/20 15:28:52]

Done.

+  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();
 }
 
 
@@ -1179,17 +1363,20 @@ void BytecodeGenerator::VisitBinaryOperation(BinaryOperation* binop) {
 
 
 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());
+  // This is a potentially two pass operation if the first pass
+  // detects loads-and-stores to locals and params used in the
+  // expression. This is hopefully rare.
+  PrepareForBinaryExpression();

[rmcilroy, 2015/10/19 12:56:22]

As mentioned, this should probably be a scope allocated type, ideally being in a
top level ExpressionResultScope as discussed offline rather than being tied to a
binary op. I'm fine with us landing this as-is for now with a TODO though to
reduce rebasing pain.

[oth, 2015/10/20 15:28:52]

Done.

+  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.
+    lhs = LoadForBinaryExpression(lhs);
+  }
+  VisitForAccumulatorValue(expr->right());
+  builder()->CompareOperation(expr->op(), lhs, language_mode_strength());
+  result_scope()->SetResultInAccumulator();
+  CompleteBinaryExpression();
 }
 
 
@@ -1281,18 +1468,82 @@ void BytecodeGenerator::VisitNewLocalBlockContext(Scope* scope) {
 }
 
 
-void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* binop) {
-  Token::Value op = binop->op();
-  Expression* left = binop->left();
-  Expression* right = binop->right();
+void BytecodeGenerator::PrepareForBinaryExpression() {
+  if (binary_expression_depth_++ == 0) {
+    binary_expression_hazard_set_.clear();
+  }
+}
+
+
+Register BytecodeGenerator::LoadForBinaryExpression(Register reg) {

[rmcilroy, 2015/10/19 12:56:22]

I think this should be done in VisitVariableLoad at the same point as we alias
the local (e.g., call this LoadLocalRegisterAsAlias to match with
RecordStoreToLocalRegister). This way it is being done close to the aliasing so
it is more obvious why it is necessary, and we won't forget to add it if there
end up being more binary op situations (e.g., CompareOps are not classed as
BinaryOps, so would have been easy to miss).

[oth, 2015/10/20 15:28:53]

The method has been renamed and there's a TODO to make this change. I'd like it
to be in the follow CL that does re-mapping.

+  // 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::StoreForBinaryExpression(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());
+  }
+}
 
-  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::CompleteBinaryExpression() {
+  DCHECK(binary_expression_depth_ > 0);
+  binary_expression_depth_ -= 1;
+  // TODO(oth): spill remapped registers into origins.
+}
+
+
+void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* expr) {
+  // This is a potentially two pass operation if the first pass
+  // detects loads-and-stores to locals and params used in the
+  // expression. This is hopefully rare.

[rmcilroy, 2015/10/19 12:56:22]

Comment is out-of-date. Alsoc could we move this back to it's original location
(putting all the Visit operators grouped and moving the
PrepareForBinaryExpression..CompleteBinaryExpression and
VisitForAccumulatorValue..VisitForRegisterValue below the visit operations.

[oth, 2015/10/20 15:28:52]

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.
+    lhs = LoadForBinaryExpression(lhs);
+  }
+  VisitForAccumulatorValue(expr->right());
+  builder()->BinaryOperation(expr->op(), lhs, language_mode_strength());
+  result_scope()->SetResultInAccumulator();
+  CompleteBinaryExpression();
+}
+
+
+// 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.
+Register BytecodeGenerator::VisitForRegisterValue(Expression* expr) {
+  RegisterResultScope register_scope(this);
+  Visit(expr);
+  return register_scope.ResultRegister();
 }
 
 
@@ -1300,8 +1551,9 @@ void BytecodeGenerator::VisitCommaExpression(BinaryOperation* binop) {
   Expression* left = binop->left();
   Expression* right = binop->right();
 
-  Visit(left);
-  Visit(right);
+  VisitForAccumulatorValue(left);

[rmcilroy, 2015/10/19 12:56:22]

this can be VisitForEffect

[oth, 2015/10/20 15:28:53]

Yes, and interestingly the RHS visitor should just be Visit() not
VisitForAccumulatorValue(). Comment in the next patch, but it's a right sided
tree and we're already in a result scope when called. If RHS is
VisitForAccumulator we still end up emitting code for all following RHS values
because need to put value in accumulator.

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

Nice catch :).

+  VisitForAccumulatorValue(right);
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -1312,15 +1564,15 @@ void BytecodeGenerator::VisitLogicalOrExpression(BinaryOperation* binop) {
   // 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();
 }
 
 
@@ -1331,15 +1583,15 @@ void BytecodeGenerator::VisitLogicalAndExpression(BinaryOperation* binop) {
   // 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();
 }
 
 
@@ -1349,7 +1601,7 @@ void BytecodeGenerator::VisitObjectLiteralAccessor(
   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);
   }