[Interpreter] Reduce temporary register usage in generated bytecode.

src/interpreter/bytecode-generator.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/interpreter/bytecode-generator.h"
 
 #include <stack>
 
 #include "src/compiler.h"
 #include "src/interpreter/control-flow-builders.h"
@@ skipping 71 matching lines @@
                                                      Statement* statement) {
   ControlScope* current = this;
   do {
     if (current->Execute(command, statement)) return;
     current = current->outer();
   } while (current != nullptr);
   UNREACHABLE();
 }
 
 
+// Scoped base class for determining where the result of an expression
+// is stored.
+class BytecodeGenerator::ExpressionResultScope {
+ public:
+  explicit ExpressionResultScope(BytecodeGenerator* generator)
+      : generator_(generator), outer_(generator->result_scope()) {
+    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);
 }
 
 
 BytecodeGenerator::~BytecodeGenerator() {}
 
 
 Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   set_info(info);
   set_scope(info->scope());
@@ skipping 119 matching lines @@
   builder()->LoadLiteral(Smi::FromInt(encoded_flags));
   builder()->StoreAccumulatorInRegister(flags);
   DCHECK(flags.index() == pairs.index() + 1);
 
   builder()->CallRuntime(Runtime::kDeclareGlobals, pairs, 2);
   globals()->clear();
 }
 
 
 void BytecodeGenerator::VisitExpressionStatement(ExpressionStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForAccumulatorValue(stmt->expression());
 }
 
 
 void BytecodeGenerator::VisitEmptyStatement(EmptyStatement* stmt) {
   // TODO(oth): For control-flow it could be useful to signal empty paths here.
 }
 
 
 void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
   // TODO(oth): Spot easy cases where there code would not need to
   // emit the then block or the else block, e.g. condition is
   // obviously true/1/false/0.
 
   BytecodeLabel else_label, end_label;
 
-  Visit(stmt->condition());
+  VisitForAccumulatorValue(stmt->condition());
   builder()->CastAccumulatorToBoolean();
   builder()->JumpIfFalse(&else_label);
   Visit(stmt->then_statement());
   if (stmt->HasElseStatement()) {
     builder()->Jump(&end_label);
     builder()->Bind(&else_label);
     Visit(stmt->else_statement());
   } else {
     builder()->Bind(&else_label);
   }
@@ skipping 11 matching lines @@
   control_scope()->Continue(stmt->target());
 }
 
 
 void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
   control_scope()->Break(stmt->target());
 }
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitSwitchStatement(SwitchStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitCaseClause(CaseClause* clause) { UNIMPLEMENTED(); }
 
 
 void BytecodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration control_scope(this, stmt, &loop_builder);
 
   BytecodeLabel body_label, condition_label, done_label;
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&condition_label);
-  Visit(stmt->cond());
+  VisitForAccumulatorValue(stmt->cond());
   builder()->JumpIfTrue(&body_label);
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(condition_label);
 }
 
 
 void BytecodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration control_scope(this, stmt, &loop_builder);
 
   BytecodeLabel body_label, condition_label, done_label;
   builder()->Jump(&condition_label);
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&condition_label);
-  Visit(stmt->cond());
+  VisitForAccumulatorValue(stmt->cond());
   builder()->JumpIfTrue(&body_label);
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(condition_label);
 }
 
 
 void BytecodeGenerator::VisitForStatement(ForStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration control_scope(this, stmt, &loop_builder);
 
   if (stmt->init() != nullptr) {
     Visit(stmt->init());
   }
 
   BytecodeLabel body_label, condition_label, next_label, done_label;
   if (stmt->cond() != nullptr) {
     builder()->Jump(&condition_label);
   }
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&next_label);
   if (stmt->next() != nullptr) {
     Visit(stmt->next());
   }
   if (stmt->cond()) {
     builder()->Bind(&condition_label);
-    Visit(stmt->cond());
+    VisitForAccumulatorValue(stmt->cond());
     builder()->JumpIfTrue(&body_label);
   } else {
     builder()->Jump(&body_label);
   }
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(next_label);
 }
 
@@ skipping 52 matching lines @@
     builder()->LoadTrue();
   } else if (value->IsFalse()) {
     builder()->LoadFalse();
   } else if (value->IsNull()) {
     builder()->LoadNull();
   } else if (value->IsTheHole()) {
     builder()->LoadTheHole();
   } else {
     builder()->LoadLiteral(value);
   }
+  result_scope()->AccumulatorSet();
 }
 
 
 void BytecodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitVariableProxy(VariableProxy* proxy) {
   VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
 }
 
 
 void BytecodeGenerator::VisitVariableLoad(Variable* variable,
                                           FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
-      Register source(variable->index());
-      builder()->LoadAccumulatorWithRegister(source);
+      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: {
       // Global var, const, or let variable.
       // TODO(rmcilroy): If context chain depth is short enough, do this using
       // a generic version of LoadGlobalViaContextStub rather than calling the
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->LoadGlobal(variable->index());
+      result_scope()->AccumulatorSet();
       break;
     }
     case VariableLocation::UNALLOCATED: {
       TemporaryRegisterScope temporary_register_scope(&builder_);
       Register obj = temporary_register_scope.NewRegister();
       builder()->LoadContextSlot(current_context(),
                                  Context::GLOBAL_OBJECT_INDEX);
       builder()->StoreAccumulatorInRegister(obj);
       builder()->LoadLiteral(variable->name());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());

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

AccumulatorSet()

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

Done.

       break;
     }
     case VariableLocation::CONTEXT:
     case VariableLocation::LOOKUP:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
                                                 FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
       // TODO(rmcilroy): support const mode initialization.
       Register destination(variable->index());
       builder()->StoreAccumulatorInRegister(destination);

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

I was chatting to Danno about this and he made a very good point where this
optimization could potentially cause incorrect semantics. If we have the
following code:

 var a = 1;
 b = a + (a = 2);

The lhs of the expression would evaluate down to a simple Register(a), and the
rhs would evaluate down to ( 2 -> Register(a), Register(a)).  Since we have
updated a to "2" in the rhs, that means the result of the add operation would be
wrong (it would be 2 + 2 rather than 1 + 2). I'm not sure how we solve this, we
probably need to check the expression stack and only do this optimization if
there are no assignment operations in any of expressions. Any thoughts?

[rmcilroy, 2015/10/08 17:00:09]

Had a further chat with Danno about this, if we keep track of the registers we
are using for an expression, then we might be able to keep this as is, but then
when we get to an assignment operation, if it is to a local / param we would
walk this list of expression registers, and if any of them are the same local /
param, then we would emit code to copy the local to a temporary register, and
use that register instead. We can chat about this tomorrow in-person. 

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

Yes, a bit of a fail not to have recalled this was an issue, but it's
definitely come up before. As discussed implementing, this is probably
best tackled in two steps since detection and failure is considerably
easier than solving the problem. The current register allocation pattern makes
life harder than necessary as it does bump allocation in nested scopes, but the
code would need the 'temporaries' to live beyond their register scope. We might
solve this by having an expression scope that subsumes the temporary register
scope functionality. Will update the review when there's a step improvement on
one of these paths.

       break;
     }
     case VariableLocation::PARAMETER: {
       // The parameter indices are shifted by 1 (receiver is variable
       // index -1 but is parameter index 0 in BytecodeArrayBuilder).
       Register destination(builder()->Parameter(variable->index() + 1));
       builder()->StoreAccumulatorInRegister(destination);
       break;
     }
     case VariableLocation::GLOBAL: {
@@ skipping 25 matching lines @@
     }
     case VariableLocation::CONTEXT:
     case VariableLocation::LOOKUP:
       UNIMPLEMENTED();
   }
 }
 
 
 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.
   Property* property = expr->target()->AsProperty();
   LhsKind assign_type = Property::GetAssignType(property);
 
   // Evaluate LHS expression.
   switch (assign_type) {
     case VARIABLE:
       // Nothing to do to evaluate variable assignment LHS.
       break;
     case NAMED_PROPERTY:
-      object = temporary_register_scope.NewRegister();
+      VisitForRegisterValue(property->obj(), &temporary_register_scope);
+      object = GetResultRegister();
+      builder()->LoadLiteral(property->key()->AsLiteral()->AsPropertyName());
       key = temporary_register_scope.NewRegister();
-      Visit(property->obj());
-      builder()->StoreAccumulatorInRegister(object);
-      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);
+      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:
       UNIMPLEMENTED();
   }
 
   // Evaluate the value and potentially handle compound assignments by loading
   // the left-hand side value and performing a binary operation.
   if (expr->is_compound()) {
     UNIMPLEMENTED();
   } else {
-    Visit(expr->value());
+    VisitForAccumulatorValue(expr->value());
   }
 
   // Store the value.
   FeedbackVectorSlot slot = expr->AssignmentSlot();
   switch (assign_type) {
     case VARIABLE: {
       Variable* variable = expr->target()->AsVariableProxy()->var();
       VisitVariableAssignment(variable, slot);
       break;
     }
@@ skipping 23 matching lines @@
   FeedbackVectorSlot slot = expr->PropertyFeedbackSlot();
   switch (property_kind) {
     case VARIABLE:
       UNREACHABLE();
     case NAMED_PROPERTY: {
       builder()->LoadLiteral(expr->key()->AsLiteral()->AsPropertyName());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());
       break;
     }
     case KEYED_PROPERTY: {
-      Visit(expr->key());
+      VisitForAccumulatorValue(expr->key());
       builder()->LoadKeyedProperty(obj, feedback_index(slot), language_mode());
       break;
     }
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitProperty(Property* expr) {
-  TemporaryRegisterScope temporary_register_scope(&builder_);
-  Register obj = temporary_register_scope.NewRegister();
-  Visit(expr->obj());
-  builder()->StoreAccumulatorInRegister(obj);
-  VisitPropertyLoad(obj, expr);
+  TemporaryRegisterScope temporary_register_scope(builder());
+  VisitForRegisterValue(expr->obj(), &temporary_register_scope);
+  VisitPropertyLoad(GetResultRegister(), expr);
 }
 
 
 void BytecodeGenerator::VisitCall(Call* expr) {
   Expression* callee_expr = expr->expression();
   Call::CallType call_type = expr->GetCallType(isolate());
 
   // Prepare the callee and the receiver to the function call. This depends on
   // the semantics of the underlying call type.
-  TemporaryRegisterScope temporary_register_scope(&builder_);
+  TemporaryRegisterScope temporary_register_scope(builder());
   Register callee = temporary_register_scope.NewRegister();
   Register receiver = temporary_register_scope.NewRegister();
 
   switch (call_type) {
     case Call::PROPERTY_CALL: {
       Property* property = callee_expr->AsProperty();
       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);
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::GLOBAL_CALL: {
       // Receiver is undefined for global calls.
       builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
       // Load callee as a global variable.
       VariableProxy* proxy = callee_expr->AsVariableProxy();
       VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::LOOKUP_SLOT_CALL:
     case Call::SUPER_CALL:
     case Call::POSSIBLY_EVAL_CALL:
     case Call::OTHER_CALL:
       UNIMPLEMENTED();
   }
 
   // Evaluate all arguments to the function call and store in sequential
   // 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);
   }
 
   // TODO(rmcilroy): Deal with possible direct eval here?
   // TODO(rmcilroy): Use CallIC to allow call type feedback.
   builder()->Call(callee, receiver, args->length());
 }
 
 
 void BytecodeGenerator::VisitCallNew(CallNew* expr) { UNIMPLEMENTED(); }
 
 
 void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
   if (expr->is_jsruntime()) {
     UNIMPLEMENTED();
   }
 
   // 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);
   }
 
   // TODO(rmcilroy): support multiple return values.
   DCHECK_LE(expr->function()->result_size, 1);
   Runtime::FunctionId function_id = expr->function()->function_id;
   builder()->CallRuntime(function_id, first_arg, args->length());
 }
 
 
 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();
 }
 
 
 void BytecodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
   switch (expr->op()) {
     case Token::Value::NOT:
       VisitNot(expr);
       break;
     case Token::Value::TYPEOF:
       VisitTypeOf(expr);
       break;
     case Token::Value::VOID:
       VisitVoid(expr);
       break;
     case Token::Value::BIT_NOT:
     case Token::Value::DELETE:
       UNIMPLEMENTED();
     default:
       UNREACHABLE();

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

AcumulatorSet ?

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

Done.

   }
 }
 
 
 void BytecodeGenerator::VisitCountOperation(CountOperation* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitBinaryOperation(BinaryOperation* binop) {
   switch (binop->op()) {
     case Token::COMMA:
     case Token::OR:
     case Token::AND:
       UNIMPLEMENTED();
       break;
     default:
       VisitArithmeticExpression(binop);
       break;
   }
 }
 
 
 void BytecodeGenerator::VisitCompareOperation(CompareOperation* expr) {
-  Token::Value op = expr->op();
-  Expression* left = expr->left();
-  Expression* right = expr->right();
-
-  TemporaryRegisterScope temporary_register_scope(&builder_);
-  Register temporary = temporary_register_scope.NewRegister();
-
-  Visit(left);
-  builder()->StoreAccumulatorInRegister(temporary);
-  Visit(right);
-  builder()->CompareOperation(op, temporary, language_mode_strength());
+  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());
 }
 
 
 void BytecodeGenerator::VisitSpread(Spread* expr) { UNREACHABLE(); }
 
 
 void BytecodeGenerator::VisitEmptyParentheses(EmptyParentheses* expr) {
   UNREACHABLE();
 }
 
 
 void BytecodeGenerator::VisitThisFunction(ThisFunction* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitSuperCallReference(SuperCallReference* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitSuperPropertyReference(
     SuperPropertyReference* expr) {
   UNIMPLEMENTED();
 }
 
 
 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());
+  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;
+}
+
+
+void BytecodeGenerator::DropResultRegister() {
+  result_register_ = Nothing<Register>();
+}
+
+
 LanguageMode BytecodeGenerator::language_mode() const {
   return info()->language_mode();
 }
 
 
 Strength BytecodeGenerator::language_mode_strength() const {
   return strength(language_mode());
 }
 
 
 int BytecodeGenerator::feedback_index(FeedbackVectorSlot slot) const {
   return info()->feedback_vector()->GetIndex(slot);
 }
 
 
 Register BytecodeGenerator::current_context() const { return current_context_; }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8