[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..9ceab67ab4f6b658a26fb3f00dc75add2b6ec3a6 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,118 @@ 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),
+        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),
@@ -154,7 +264,10 @@ BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
       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);
 }
 
@@ -324,12 +437,11 @@ void BytecodeGenerator::VisitDeclarations(
 
 
 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.
 }
 
 
@@ -340,7 +452,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 +484,7 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
@@ -398,7 +510,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 +528,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 +557,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 +612,7 @@ void BytecodeGenerator::VisitFunctionLiteral(FunctionLiteral* expr) {
   builder()
       ->LoadLiteral(shared_info)
       .CreateClosure(expr->pretenure() ? TENURED : NOT_TENURED);
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -534,6 +647,7 @@ void BytecodeGenerator::VisitLiteral(Literal* expr) {
   } else {
     builder()->LoadLiteral(value);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -546,6 +660,7 @@ void BytecodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
       .StoreAccumulatorInRegister(flags)
       .LoadLiteral(expr->pattern())
       .CreateRegExpLiteral(expr->literal_index(), flags);
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -590,21 +705,28 @@ 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());
+            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()
@@ -617,10 +739,12 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
         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;
@@ -643,12 +767,13 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   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);
@@ -671,7 +796,8 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
     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;
     }
@@ -681,21 +807,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());
-    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);
@@ -729,6 +856,7 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -760,11 +888,11 @@ void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
       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());
   }
@@ -773,6 +901,7 @@ void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -785,17 +914,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 +932,7 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->LoadGlobal(variable->index());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::UNALLOCATED: {
@@ -815,6 +943,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 +953,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 +971,7 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
       // TODO(rmcilroy): support const mode initialization.
       Register destination(variable->index());
       builder()->StoreAccumulatorInRegister(destination);
+      RecordStoreToRegister(destination);
       break;
     }
     case VariableLocation::PARAMETER: {
@@ -848,6 +979,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);
+      RecordStoreToRegister(destination);
       break;
     }
     case VariableLocation::GLOBAL: {
@@ -860,10 +992,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 +1027,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 +1038,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 +1060,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 +1102,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,29 +1119,28 @@ 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));
+  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;
 }
 
@@ -1022,9 +1151,14 @@ void BytecodeGenerator::VisitCall(Call* expr) {
 
   // 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: {
@@ -1032,14 +1166,18 @@ 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);
       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.
@@ -1050,7 +1188,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 +1200,26 @@ 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);
+    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
@@ -1089,6 +1227,8 @@ void BytecodeGenerator::VisitCallNew(CallNew* expr) {
     // allocating a temporary just to fill the operands.
     builder()->New(constructor, constructor, 0);
   }
+
+  result_scope()->SetResultInAccumulator();
 }
 
 
@@ -1097,41 +1237,44 @@ void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
     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();
 }
 
 
@@ -1179,17 +1322,40 @@ 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());
+  // 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();
 }
 
 
@@ -1281,27 +1447,9 @@ void BytecodeGenerator::VisitNewLocalBlockContext(Scope* scope) {
 }
 
 
-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());
 }
 
 
@@ -1312,15 +1460,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 +1479,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 +1497,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);
   }
@@ -1393,6 +1541,68 @@ void BytecodeGenerator::VisitFunctionClosureForContext() {
 }
 
 
+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.