[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 423c438cf2c17910227d08fd4b873294538d2dbb..10db07386bdc8a83affa2b6b2859fdc2d59318fd 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -89,13 +89,78 @@ 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()) {
+    generator_->set_result_scope(this);
+  }
+
+  virtual ~ExpressionResultScope() { generator_->set_result_scope(outer_); }
+
+  virtual void AccumulatorSet() = 0;
+  virtual void RegisterSet(Register reg) = 0;

[rmcilroy, 2015/10/08 16:54:26]

Not sure I like these names. How about SetResultInAccumulator() and
SetResultInRegister().

[oth, 2015/10/09 12:50:48]

Done.

+
+  BytecodeGenerator* generator() const { return generator_; }
+  BytecodeArrayBuilder* builder() const { return generator()->builder(); }
+
+ private:
+  BytecodeGenerator* generator_;
+  ExpressionResultScope* outer_;
+  DISALLOW_COPY_AND_ASSIGN(ExpressionResultScope);
+};
+
+
+// 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 AccumulatorSet() {}
+  virtual void RegisterSet(Register reg) {
+    builder()->LoadAccumulatorWithRegister(reg);

[rmcilroy, 2015/10/08 16:54:26]

As discussed offline, could we add a DCHECK to make sure that 1 and only 1 of
these gets called on a given Expression scope.

[oth, 2015/10/09 12:50:48]

Done.

+  }
+};
+
+
+// Scoped class used when the result of the current expression to be
+// evaluated should go into an interpreter register. The register
+// selected is then available when GetResultRegister() is called.
+class BytecodeGenerator::RegisterResultScope final
+    : public ExpressionResultScope {
+ public:
+  explicit RegisterResultScope(BytecodeGenerator* generator,
+                                   TemporaryRegisterScope* register_scope)
+      : ExpressionResultScope(generator), register_scope_(register_scope) {}
+
+  virtual void AccumulatorSet() {
+    Register allocated = register_scope_->NewRegister();
+    builder()->StoreAccumulatorInRegister(allocated);
+    generator()->SetResultRegister(allocated);
+  }
+
+  virtual void RegisterSet(Register reg) {
+    generator()->SetResultRegister(reg);
+  }
+
+ private:
+  TemporaryRegisterScope* register_scope_;
+};
+

[rmcilroy, 2015/10/08 16:54:26]

As discussed offline, could we also add a NullResultScope which would DCHECK if
either AccumulatorSet or RegisterSet are called, and is used when calling a
normal "Visit" (we will presumably need to override Visit to do this).

[oth, 2015/10/09 12:50:48]

If the ast visitor is not in an expression then the result scope is null so
SetResultInAccumulator/SetResultInRegister fail.

The normal visit is already implemented by DEFINE_AST_VISITOR_SUBCLASS_MEMBERS
so if there's additional value on this path we'd need to drop the definition in
the macro and push the change elsewhere.

+
 BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
     : builder_(isolate, zone),
       info_(nullptr),
       scope_(nullptr),
       globals_(0, zone),
       control_scope_(nullptr),
-      current_context_(Register::function_context()) {
+      current_context_(Register::function_context()),
+      result_scope_(nullptr),
+      result_register_(Nothing<Register>()) {
   InitializeAstVisitor(isolate, zone);
 }
 
@@ -235,7 +300,7 @@ void BytecodeGenerator::VisitDeclarations(
 
 
 void BytecodeGenerator::VisitExpressionStatement(ExpressionStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForAccumulatorValue(stmt->expression());
 }
 
 
@@ -251,7 +316,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());
@@ -283,7 +348,7 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
@@ -309,7 +374,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);
 
@@ -327,7 +392,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);
 
@@ -356,7 +421,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);
@@ -429,6 +494,7 @@ void BytecodeGenerator::VisitLiteral(Literal* expr) {
   } else {
     builder()->LoadLiteral(value);
   }
+  result_scope()->AccumulatorSet();
 }
 
 
@@ -456,15 +522,13 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
                                           FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
-      Register source(variable->index());
-      builder()->LoadAccumulatorWithRegister(source);
+      result_scope()->RegisterSet(Register(variable->index()));
       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);
+      result_scope()->RegisterSet(builder()->Parameter(variable->index() + 1));
       break;
     }
     case VariableLocation::GLOBAL: {
@@ -474,6 +538,7 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->LoadGlobal(variable->index());
+      result_scope()->AccumulatorSet();
       break;
     }
     case VariableLocation::UNALLOCATED: {
@@ -545,7 +610,7 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
 
 void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   DCHECK(expr->target()->IsValidReferenceExpression());
-  TemporaryRegisterScope temporary_register_scope(&builder_);
+  TemporaryRegisterScope temporary_register_scope(builder());
   Register object, key;
 
   // Left-hand side can only be a property, a global or a variable slot.
@@ -558,20 +623,17 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
       // 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);
+      VisitForRegisterValue(property->obj(), &temporary_register_scope);
+      object = GetResultRegister();
       builder()->LoadLiteral(property->key()->AsLiteral()->AsPropertyName());
+      key = temporary_register_scope.NewRegister();
       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);
+      VisitForRegisterValue(property->obj(), &temporary_register_scope);
+      object = GetResultRegister();
+      VisitForRegisterValue(property->key(), &temporary_register_scope);
+      key = GetResultRegister();
       break;
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
@@ -583,7 +645,7 @@ void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   if (expr->is_compound()) {
     UNIMPLEMENTED();
   } else {
-    Visit(expr->value());
+    VisitForAccumulatorValue(expr->value());
   }
 
   // Store the value.
@@ -627,7 +689,7 @@ void BytecodeGenerator::VisitPropertyLoad(Register obj, Property* expr) {
       break;
     }
     case KEYED_PROPERTY: {
-      Visit(expr->key());
+      VisitForAccumulatorValue(expr->key());
       builder()->LoadKeyedProperty(obj, feedback_index(slot), language_mode());
       break;
     }
@@ -639,11 +701,9 @@ 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);
-  VisitPropertyLoad(obj, expr);
+  TemporaryRegisterScope temporary_register_scope(builder());
+  VisitForRegisterValue(expr->obj(), &temporary_register_scope);
+  VisitPropertyLoad(GetResultRegister(), expr);
 }
 
 
@@ -653,7 +713,7 @@ 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_);
+  TemporaryRegisterScope temporary_register_scope(builder());
   Register callee = temporary_register_scope.NewRegister();
   Register receiver = temporary_register_scope.NewRegister();
 
@@ -663,7 +723,9 @@ void BytecodeGenerator::VisitCall(Call* expr) {
       if (property->IsSuperAccess()) {
         UNIMPLEMENTED();
       }
-      Visit(property->obj());
+      VisitForAccumulatorValue(property->obj());
+      // Receiver needs to explicitly be in a register, cannot be
+      // a param. Hence explicit allocation and store.

[rmcilroy, 2015/10/08 16:54:25]

If there were no other arguments (or all the arguments happened to be sequential
params or locals) then it would actually be possible to use a parameter or local
registers (and might be a nice optimization for wrapper functions which call
another function with the same arguments). Definitely a TODO for future work
though :).

Could you clarify the comment saying the "Receiver needs to be in a temporary
register so that arguments can be in subsequent ordered registers".

[oth, 2015/10/09 12:50:48]

Done.

       builder()->StoreAccumulatorInRegister(receiver);
       // Perform a property load of the callee.
       VisitPropertyLoad(receiver, property);
@@ -690,7 +752,7 @@ void BytecodeGenerator::VisitCall(Call* expr) {
   // registers.
   ZoneList<Expression*>* args = expr->arguments();
   for (int i = 0; i < args->length(); ++i) {
-    Visit(args->at(i));
+    VisitForAccumulatorValue(args->at(i));
     Register arg = temporary_register_scope.NewRegister();
     DCHECK(arg.index() - i == receiver.index() + 1);
     builder()->StoreAccumulatorInRegister(arg);
@@ -712,14 +774,14 @@ void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
 
   // Evaluate all arguments to the runtime call.
   ZoneList<Expression*>* args = expr->arguments();
-  TemporaryRegisterScope temporary_register_scope(&builder_);
+  TemporaryRegisterScope temporary_register_scope(builder());
   // Ensure we always have a valid first_arg register even if there are no
   // arguments to pass.
   Register first_arg = temporary_register_scope.NewRegister();
   for (int i = 0; i < args->length(); ++i) {
     Register arg =
         (i == 0) ? first_arg : temporary_register_scope.NewRegister();
-    Visit(args->at(i));
+    VisitForAccumulatorValue(args->at(i));
     DCHECK_EQ(arg.index() - i, first_arg.index());
     builder()->StoreAccumulatorInRegister(arg);
   }
@@ -732,19 +794,19 @@ void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
 
 
 void BytecodeGenerator::VisitVoid(UnaryOperation* expr) {
-  Visit(expr->expression());
+  VisitForAccumulatorValue(expr->expression());
   builder()->LoadUndefined();
 }
 
 
 void BytecodeGenerator::VisitTypeOf(UnaryOperation* expr) {
-  Visit(expr->expression());
+  VisitForAccumulatorValue(expr->expression());
   builder()->TypeOf();
 }
 
 
 void BytecodeGenerator::VisitNot(UnaryOperation* expr) {
-  Visit(expr->expression());
+  VisitForAccumulatorValue(expr->expression());
   builder()->LogicalNot();
 }
 
@@ -789,17 +851,11 @@ 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());
+  TemporaryRegisterScope temporary_register_scope(builder());
+  VisitForRegisterValue(expr->left(), &temporary_register_scope);
+  Register lhs = GetResultRegister();
+  VisitForAccumulatorValue(expr->right());
+  builder()->CompareOperation(expr->op(), lhs, language_mode_strength());
 }
 
 
@@ -828,17 +884,44 @@ void BytecodeGenerator::VisitSuperPropertyReference(
 
 
 void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* binop) {
-  Token::Value op = binop->op();
-  Expression* left = binop->left();
-  Expression* right = binop->right();
+  TemporaryRegisterScope temporary_register_scope(builder());
+  VisitForRegisterValue(binop->left(), &temporary_register_scope);
+  Register lhs = GetResultRegister();
+  VisitForAccumulatorValue(binop->right());
+  builder()->BinaryOperation(binop->op(), lhs, language_mode_strength());
+  result_scope()->AccumulatorSet();
+}
+
+
+void BytecodeGenerator::VisitForRegisterValue(
+    Expression* expr, TemporaryRegisterScope* temporary_register_scope) {
+  RegisterResultScope register_scope(this, temporary_register_scope);
+  Visit(expr);
+}
+
+
+void BytecodeGenerator::VisitForAccumulatorValue(Expression* expr) {
+  AccumulatorResultScope accumulator_scope(this);
+  Visit(expr);
+}
+
+
+void BytecodeGenerator::SetResultRegister(Register reg) {
+  DCHECK(result_register_.IsNothing());
+  result_register_ = Just<Register>(reg);
+}
+
+
+Register BytecodeGenerator::GetResultRegister() {
+  DCHECK(result_register_.IsJust());
+  Register reg = result_register_.FromJust();
+  DropResultRegister();
+  return reg;
+}
 
-  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::DropResultRegister() {
+  result_register_ = Nothing<Register>();
 }