[Interpreter] Reduce temporary register usage in generated bytecode.

src/interpreter/bytecode-generator.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/interpreter/bytecode-generator.h"
 
-#include <stack>
-
 #include "src/compiler.h"
 #include "src/interpreter/control-flow-builders.h"
 #include "src/objects.h"
 #include "src/parser.h"
 #include "src/scopes.h"
 #include "src/token.h"
 
 namespace v8 {
 namespace internal {
 namespace interpreter {
@@ skipping 120 matching lines @@
                                                      Statement* statement) {
   ControlScope* current = this;
   do {
     if (current->Execute(command, statement)) return;
     current = current->outer();
   } while (current != nullptr);
   UNREACHABLE();
 }
 
 
+// Scoped base class for determining where the result of an expression
+// is stored.
+class BytecodeGenerator::ExpressionResultScope {
+ public:
+  explicit ExpressionResultScope(BytecodeGenerator* generator)
+      : generator_(generator),
+        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),
       builder_(isolate, zone),
       info_(nullptr),
       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);
 }
 
 
 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());
 
   // Initialize the incoming context.
   ContextScope incoming_context(this, scope(), false);
 
   builder()->set_parameter_count(info->num_parameters_including_this());
   builder()->set_locals_count(scope()->num_stack_slots());
   builder()->set_context_count(scope()->MaxNestedContextChainLength());
@@ skipping 142 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());
+  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.
 }
 
 
 void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
   // TODO(oth): Spot easy cases where there code would not need to
   // emit the then block or the else block, e.g. condition is
   // obviously true/1/false/0.
 
   BytecodeLabel else_label, end_label;
 
-  Visit(stmt->condition());
+  VisitForAccumulatorValue(stmt->condition());
   builder()->CastAccumulatorToBoolean();
   builder()->JumpIfFalse(&else_label);
   Visit(stmt->then_statement());
   if (stmt->HasElseStatement()) {
     builder()->Jump(&end_label);
     builder()->Bind(&else_label);
     Visit(stmt->else_statement());
   } else {
     builder()->Bind(&else_label);
   }
@@ skipping 11 matching lines @@
   execution_control()->Continue(stmt->target());
 }
 
 
 void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
   execution_control()->Break(stmt->target());
 }
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  Visit(stmt->expression());
+  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();
 }
 
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitSwitchStatement(SwitchStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitCaseClause(CaseClause* clause) { UNIMPLEMENTED(); }
 
 
 void BytecodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
 
   BytecodeLabel body_label, condition_label, done_label;
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&condition_label);
-  Visit(stmt->cond());
+  VisitForAccumulatorValue(stmt->cond());
   builder()->JumpIfTrue(&body_label);
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(condition_label);
 }
 
 
 void BytecodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
 
   BytecodeLabel body_label, condition_label, done_label;
   builder()->Jump(&condition_label);
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&condition_label);
-  Visit(stmt->cond());
+  VisitForAccumulatorValue(stmt->cond());
   builder()->JumpIfTrue(&body_label);
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(condition_label);
 }
 
 
 void BytecodeGenerator::VisitForStatement(ForStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
 
   if (stmt->init() != nullptr) {
     Visit(stmt->init());
   }
 
   BytecodeLabel body_label, condition_label, next_label, done_label;
   if (stmt->cond() != nullptr) {
     builder()->Jump(&condition_label);
   }
   builder()->Bind(&body_label);
   Visit(stmt->body());
   builder()->Bind(&next_label);
   if (stmt->next() != nullptr) {
     Visit(stmt->next());
   }
   if (stmt->cond()) {
     builder()->Bind(&condition_label);
-    Visit(stmt->cond());
+    VisitForAccumulatorValue(stmt->cond());
     builder()->JumpIfTrue(&body_label);
   } else {
     builder()->Jump(&body_label);
   }
   builder()->Bind(&done_label);
 
   loop_builder.SetBreakTarget(done_label);
   loop_builder.SetContinueTarget(next_label);
 }
 
@@ skipping 34 matching lines @@
 
 void BytecodeGenerator::VisitFunctionLiteral(FunctionLiteral* expr) {
   // Find or build a shared function info.
   Handle<SharedFunctionInfo> shared_info =
       Compiler::GetSharedFunctionInfo(expr, info()->script(), info());
   CHECK(!shared_info.is_null());  // TODO(rmcilroy): Set stack overflow?
 
   builder()
       ->LoadLiteral(shared_info)
       .CreateClosure(expr->pretenure() ? TENURED : NOT_TENURED);
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitClassLiteral(ClassLiteral* expr) {
   UNIMPLEMENTED();
 }
 
 
 void BytecodeGenerator::VisitNativeFunctionLiteral(
     NativeFunctionLiteral* expr) {
@@ skipping 14 matching lines @@
     builder()->LoadTrue();
   } else if (value->IsFalse()) {
     builder()->LoadFalse();
   } else if (value->IsNull()) {
     builder()->LoadNull();
   } else if (value->IsTheHole()) {
     builder()->LoadTheHole();
   } else {
     builder()->LoadLiteral(value);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitRegExpLiteral(RegExpLiteral* expr) {
   // Materialize a regular expression literal.
   TemporaryRegisterScope temporary_register_scope(builder());
   Register flags = temporary_register_scope.NewRegister();
   builder()
       ->LoadLiteral(expr->flags())
       .StoreAccumulatorInRegister(flags)
       .LoadLiteral(expr->pattern())
       .CreateRegExpLiteral(expr->literal_index(), flags);
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
   // Deep-copy the literal boilerplate.
   builder()
       ->LoadLiteral(expr->constant_properties())
       .CreateObjectLiteral(expr->literal_index(), expr->ComputeFlags(true));
 
   TemporaryRegisterScope temporary_register_scope(builder());
@@ skipping 24 matching lines @@
       // Fall through.
       case ObjectLiteral::Property::COMPUTED: {
         // It is safe to use [[Put]] here because the boilerplate already
         // contains computed properties with an uninitialized value.
         if (literal_key->value()->IsInternalizedString()) {
           if (property->emit_store()) {
             Register name = inner_temporary_register_scope.NewRegister();
             builder()
                 ->LoadLiteral(literal_key->AsPropertyName())
                 .StoreAccumulatorInRegister(name);
-            Visit(property->value());
+            VisitForAccumulatorValue(property->value());
             builder()->StoreNamedProperty(literal, name,
                                           feedback_index(property->GetSlot(0)),
                                           language_mode());
           } else {
-            Visit(property->value());
+            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()
                 ->LoadLiteral(Smi::FromInt(SLOPPY))
                 .StoreAccumulatorInRegister(language)
                 .CallRuntime(Runtime::kSetProperty, literal, 4);
             VisitSetHomeObject(value, literal, property);
           }
         }
         break;
       }
       case ObjectLiteral::Property::PROTOTYPE: {
         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;
       }
       case ObjectLiteral::Property::GETTER:
         if (property->emit_store()) {
           accessor_table.lookup(literal_key)->second->getter = property;
         }
         break;
       case ObjectLiteral::Property::SETTER:
         if (property->emit_store()) {
           accessor_table.lookup(literal_key)->second->setter = property;
         }
         break;
     }
   }
 
   // Define accessors, using only a single call to the runtime for each pair of
   // corresponding getters and setters.
   for (AccessorTable::Iterator it = accessor_table.begin();
        it != accessor_table.end(); ++it) {
     TemporaryRegisterScope inner_temporary_register_scope(builder());
     Register name = inner_temporary_register_scope.NewRegister();
     Register getter = inner_temporary_register_scope.NewRegister();
     Register setter = inner_temporary_register_scope.NewRegister();
     Register attr = inner_temporary_register_scope.NewRegister();
     DCHECK(Register::AreContiguous(literal, name, getter, setter, attr));
-    Visit(it->first);
+    VisitForAccumulatorValue(it->first);
     builder()->StoreAccumulatorInRegister(name);
     VisitObjectLiteralAccessor(literal, it->second->getter, getter);
     VisitObjectLiteralAccessor(literal, it->second->setter, setter);
     builder()
         ->LoadLiteral(Smi::FromInt(NONE))
         .StoreAccumulatorInRegister(attr)
         .CallRuntime(Runtime::kDefineAccessorPropertyUnchecked, literal, 5);
   }
 
   // Object literals have two parts. The "static" part on the left contains no
   // computed property names, and so we can compute its map ahead of time; see
   // Runtime_CreateObjectLiteralBoilerplate. The second "dynamic" part starts
   // with the first computed property name and continues with all properties to
   // its right. All the code from above initializes the static component of the
   // object literal, and arranges for the map of the result to reflect the
   // static order in which the keys appear. For the dynamic properties, we
   // compile them into a series of "SetOwnProperty" runtime calls. This will
   // preserve insertion order.
   for (; property_index < expr->properties()->length(); property_index++) {
     ObjectLiteral::Property* property = expr->properties()->at(property_index);
 
     if (literal_in_accumulator) {
+      temporary_register_scope.PrepareForConsecutiveAllocations(4);
       literal = temporary_register_scope.NewRegister();
       builder()->StoreAccumulatorInRegister(literal);
       literal_in_accumulator = false;
     }
 
     if (property->kind() == ObjectLiteral::Property::PROTOTYPE) {
       DCHECK(property->emit_store());
       TemporaryRegisterScope inner_temporary_register_scope(builder());
       Register value = inner_temporary_register_scope.NewRegister();
       DCHECK(Register::AreContiguous(literal, value));
-      Visit(property->value());
+      VisitForAccumulatorValue(property->value());
       builder()->StoreAccumulatorInRegister(value).CallRuntime(
           Runtime::kInternalSetPrototype, literal, 2);
       continue;
     }
 
     TemporaryRegisterScope inner_temporary_register_scope(builder());
+    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);
     Runtime::FunctionId function_id = static_cast<Runtime::FunctionId>(-1);
     switch (property->kind()) {
       case ObjectLiteral::Property::CONSTANT:
       case ObjectLiteral::Property::COMPUTED:
       case ObjectLiteral::Property::MATERIALIZED_LITERAL:
         function_id = Runtime::kDefineDataPropertyUnchecked;
         break;
@@ skipping 13 matching lines @@
   // Transform literals that contain functions to fast properties.
   if (expr->has_function()) {
     DCHECK(!literal_in_accumulator);
     builder()->CallRuntime(Runtime::kToFastProperties, literal, 1);
   }
 
   if (!literal_in_accumulator) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitArrayLiteral(ArrayLiteral* expr) {
   // Deep-copy the literal boilerplate.
   builder()
       ->LoadLiteral(expr->constant_elements())
       .CreateArrayLiteral(expr->literal_index(), expr->ComputeFlags(true));
 
   TemporaryRegisterScope temporary_register_scope(builder());
@@ skipping 14 matching lines @@
     if (literal_in_accumulator) {
       index = temporary_register_scope.NewRegister();
       literal = temporary_register_scope.NewRegister();
       builder()->StoreAccumulatorInRegister(literal);
       literal_in_accumulator = false;
     }
 
     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());
   }
 
   if (!literal_in_accumulator) {
     // Restore literal array into accumulator.
     builder()->LoadAccumulatorWithRegister(literal);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitVariableProxy(VariableProxy* proxy) {
   VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
 }
 
 
 void BytecodeGenerator::VisitVariableLoad(Variable* variable,
                                           FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
-      Register source(variable->index());
-      builder()->LoadAccumulatorWithRegister(source);
-      // TODO(rmcilroy): Perform check for uninitialized legacy const, const and
-      // let variables.
+      Register source(Register(variable->index()));
+      result_scope()->SetResultInRegister(source);
       break;
     }
     case VariableLocation::PARAMETER: {
       // The parameter indices are shifted by 1 (receiver is variable
       // index -1 but is parameter index 0 in BytecodeArrayBuilder).
-      Register source(builder()->Parameter(variable->index() + 1));
-      builder()->LoadAccumulatorWithRegister(source);
+      Register source = builder()->Parameter(variable->index() + 1);
+      result_scope()->SetResultInRegister(source);
       break;
     }
     case VariableLocation::GLOBAL: {
       // Global var, const, or let variable.
       // TODO(rmcilroy): If context chain depth is short enough, do this using
       // a generic version of LoadGlobalViaContextStub rather than calling the
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->LoadGlobal(variable->index());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::UNALLOCATED: {
       TemporaryRegisterScope temporary_register_scope(builder());
       Register obj = temporary_register_scope.NewRegister();
       builder()->LoadContextSlot(execution_context()->reg(),
                                  Context::GLOBAL_OBJECT_INDEX);
       builder()->StoreAccumulatorInRegister(obj);
       builder()->LoadLiteral(variable->name());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case VariableLocation::CONTEXT: {
       ContextScope* context = execution_context()->Previous(variable->scope());
       if (context) {
         builder()->LoadContextSlot(context->reg(), variable->index());
       } else {
         UNIMPLEMENTED();
       }
+      result_scope()->SetResultInAccumulator();
       // TODO(rmcilroy): Perform check for uninitialized legacy const, const and
       // let variables.
       break;
     }
     case VariableLocation::LOOKUP:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
                                                 FeedbackVectorSlot slot) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
       // TODO(rmcilroy): support const mode initialization.
       Register destination(variable->index());
       builder()->StoreAccumulatorInRegister(destination);
+      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: {
       // 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);
+      StoreForBinaryExpression(destination);
       break;
     }
     case VariableLocation::GLOBAL: {
       // Global var, const, or let variable.
       // TODO(rmcilroy): If context chain depth is short enough, do this using
       // a generic version of LoadGlobalViaContextStub rather than calling the
       // runtime.
       DCHECK(variable->IsStaticGlobalObjectProperty());
       builder()->StoreGlobal(variable->index(), language_mode());
       break;
     }
     case VariableLocation::UNALLOCATED: {
-      TemporaryRegisterScope temporary_register_scope(builder());
-      Register value = temporary_register_scope.NewRegister();
-      Register obj = temporary_register_scope.NewRegister();
-      Register name = temporary_register_scope.NewRegister();
+      Register value = result_scope()->NewRegister();
+      Register obj = result_scope()->NewRegister();
+      Register name = result_scope()->NewRegister();
+
       // TODO(rmcilroy): Investigate whether we can avoid having to stash the
       // value in a register.
       builder()->StoreAccumulatorInRegister(value);
       builder()->LoadContextSlot(execution_context()->reg(),
                                  Context::GLOBAL_OBJECT_INDEX);
       builder()->StoreAccumulatorInRegister(obj);
       builder()->LoadLiteral(variable->name());
       builder()->StoreAccumulatorInRegister(name);
       builder()->LoadAccumulatorWithRegister(value);
       builder()->StoreNamedProperty(obj, name, feedback_index(slot),
@@ skipping 11 matching lines @@
       break;
     }
     case VariableLocation::LOOKUP:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitAssignment(Assignment* expr) {
   DCHECK(expr->target()->IsValidReferenceExpression());
-  TemporaryRegisterScope temporary_register_scope(builder());
   Register object, key;
 
   // Left-hand side can only be a property, a global or a variable slot.
   Property* property = expr->target()->AsProperty();
   LhsKind assign_type = Property::GetAssignType(property);
 
   // Evaluate LHS expression.
   switch (assign_type) {
     case VARIABLE:
       // Nothing to do to evaluate variable assignment LHS.
       break;
-    case NAMED_PROPERTY:
-      object = temporary_register_scope.NewRegister();
-      key = temporary_register_scope.NewRegister();
-      Visit(property->obj());
-      builder()->StoreAccumulatorInRegister(object);
+    case NAMED_PROPERTY: {
+      object = VisitForRegisterValue(property->obj());
+      key = result_scope()->NewRegister();
       builder()->LoadLiteral(property->key()->AsLiteral()->AsPropertyName());
       builder()->StoreAccumulatorInRegister(key);
       break;
-    case KEYED_PROPERTY:
-      object = temporary_register_scope.NewRegister();
-      key = temporary_register_scope.NewRegister();
-      Visit(property->obj());
-      builder()->StoreAccumulatorInRegister(object);
-      Visit(property->key());
-      builder()->StoreAccumulatorInRegister(key);
+    }
+    case KEYED_PROPERTY: {
+      object = VisitForRegisterValue(property->obj());
+      key = VisitForRegisterValue(property->key());
       break;
+    }
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
       UNIMPLEMENTED();
   }
 
   // Evaluate the value and potentially handle compound assignments by loading
   // the left-hand side value and performing a binary operation.
   if (expr->is_compound()) {
     UNIMPLEMENTED();
   } else {
-    Visit(expr->value());
+    VisitForAccumulatorValue(expr->value());
+    result_scope()->SetResultInAccumulator();
   }
 
   // Store the value.
   FeedbackVectorSlot slot = expr->AssignmentSlot();
   switch (assign_type) {
     case VARIABLE: {
       Variable* variable = expr->target()->AsVariableProxy()->var();
       VisitVariableAssignment(variable, slot);
       break;
     }
@@ skipping 20 matching lines @@
 
 void BytecodeGenerator::VisitPropertyLoad(Register obj, Property* expr) {
   LhsKind property_kind = Property::GetAssignType(expr);
   FeedbackVectorSlot slot = expr->PropertyFeedbackSlot();
   switch (property_kind) {
     case VARIABLE:
       UNREACHABLE();
     case NAMED_PROPERTY: {
       builder()->LoadLiteral(expr->key()->AsLiteral()->AsPropertyName());
       builder()->LoadNamedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case KEYED_PROPERTY: {
-      Visit(expr->key());
+      VisitForAccumulatorValue(expr->key());
       builder()->LoadKeyedProperty(obj, feedback_index(slot), language_mode());
+      result_scope()->SetResultInAccumulator();
       break;
     }
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitProperty(Property* expr) {
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register obj = temporary_register_scope.NewRegister();
-  Visit(expr->obj());
-  builder()->StoreAccumulatorInRegister(obj);
+  Register obj = VisitForRegisterValue(expr->obj());
   VisitPropertyLoad(obj, expr);
 }
 
 
-Register BytecodeGenerator::VisitArguments(
-    ZoneList<Expression*>* args, TemporaryRegisterScope* register_scope) {
+Register BytecodeGenerator::VisitArguments(ZoneList<Expression*>* args) {
   // Visit arguments and place in a contiguous block of temporary registers.
   // Return the first temporary register corresponding to the first argument.
   DCHECK_GT(args->length(), 0);
-  Register first_arg = register_scope->NewRegister();
-  Visit(args->at(0));
+  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());
   }
 
   return first_arg;
 }
 
 
 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: {
       Property* property = callee_expr->AsProperty();
       if (property->IsSuperAccess()) {
         UNIMPLEMENTED();
       }
-      Visit(property->obj());
+      VisitForAccumulatorValue(property->obj());
       builder()->StoreAccumulatorInRegister(receiver);
+      // Need a result scope here to keep our consecutive
+      // temporaries.
+      AccumulatorResultScope accumulator_result_scope(this);
       // Perform a property load of the callee.
       VisitPropertyLoad(receiver, property);
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::GLOBAL_CALL: {
       // Receiver is undefined for global calls.
       builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
       // Load callee as a global variable.
       VariableProxy* proxy = callee_expr->AsVariableProxy();
       VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::OTHER_CALL: {
       builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
-      Visit(callee_expr);
+      VisitForAccumulatorValue(callee_expr);
       builder()->StoreAccumulatorInRegister(callee);
       break;
     }
     case Call::LOOKUP_SLOT_CALL:
     case Call::SUPER_CALL:
     case Call::POSSIBLY_EVAL_CALL:
       UNIMPLEMENTED();
   }
 
   // Evaluate all arguments to the function call and store in sequential
   // registers.
-  ZoneList<Expression*>* args = expr->arguments();
-  if (args->length() > 0) {
-    Register first_arg = VisitArguments(args, &temporary_register_scope);
-    CHECK_EQ(first_arg.index(), receiver.index() + 1);
+  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
     // arguments. Using the constructor register avoids avoid
     // allocating a temporary just to fill the operands.
     builder()->New(constructor, constructor, 0);
   }
+
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitCallRuntime(CallRuntime* expr) {
   if (expr->is_jsruntime()) {
     UNIMPLEMENTED();
   }
 
   // Evaluate all arguments to the runtime call.
-  TemporaryRegisterScope temporary_register_scope(&builder_);
-
-  // TODO(rmcilroy): support multiple return values.
   DCHECK_LE(expr->function()->result_size, 1);
   Runtime::FunctionId function_id = expr->function()->function_id;
   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();
 }
 
 
 void BytecodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
   switch (expr->op()) {
     case Token::Value::NOT:
       VisitNot(expr);
       break;
     case Token::Value::TYPEOF:
       VisitTypeOf(expr);
@@ skipping 27 matching lines @@
       VisitLogicalAndExpression(binop);
       break;
     default:
       VisitArithmeticExpression(binop);
       break;
   }
 }
 
 
 void BytecodeGenerator::VisitCompareOperation(CompareOperation* expr) {
-  Token::Value op = expr->op();
-  Expression* left = expr->left();
-  Expression* right = expr->right();
-
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register temporary = temporary_register_scope.NewRegister();
-
-  Visit(left);
-  builder()->StoreAccumulatorInRegister(temporary);
-  Visit(right);
-  builder()->CompareOperation(op, temporary, language_mode_strength());
+  // 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();
 }
 
 
 void BytecodeGenerator::VisitSpread(Spread* expr) { UNREACHABLE(); }
 
 
 void BytecodeGenerator::VisitEmptyParentheses(EmptyParentheses* expr) {
   UNREACHABLE();
 }
 
@@ skipping 71 matching lines @@
   builder()
       ->LoadLiteral(scope->GetScopeInfo(isolate()))
       .StoreAccumulatorInRegister(scope_info);
   VisitFunctionClosureForContext();
   builder()
       ->StoreAccumulatorInRegister(closure)
       .CallRuntime(Runtime::kPushBlockContext, scope_info, 2);
 }
 
 
-void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* binop) {
-  Token::Value op = binop->op();
-  Expression* left = binop->left();
-  Expression* right = binop->right();
-
-  TemporaryRegisterScope temporary_register_scope(builder());
-  Register temporary = temporary_register_scope.NewRegister();
-
-  Visit(left);
-  builder()->StoreAccumulatorInRegister(temporary);
-  Visit(right);
-  builder()->BinaryOperation(op, temporary, language_mode_strength());
+void BytecodeGenerator::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());
+  }
+}
+
+
+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();
+}
+
+
 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();
 }
 
 
 void BytecodeGenerator::VisitLogicalOrExpression(BinaryOperation* binop) {
   Expression* left = binop->left();
   Expression* right = binop->right();
 
   // Short-circuit evaluation- If it is known that left is always true,
   // no need to visit right
   if (left->ToBooleanIsTrue()) {
-    Visit(left);
+    VisitForAccumulatorValue(left);
   } else {
     BytecodeLabel end_label;
-
-    Visit(left);
+    VisitForAccumulatorValue(left);
     builder()->JumpIfToBooleanTrue(&end_label);
-    Visit(right);
+    VisitForAccumulatorValue(right);
     builder()->Bind(&end_label);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitLogicalAndExpression(BinaryOperation* binop) {
   Expression* left = binop->left();
   Expression* right = binop->right();
 
   // Short-circuit evaluation- If it is known that left is always false,
   // no need to visit right
   if (left->ToBooleanIsFalse()) {
-    Visit(left);
+    VisitForAccumulatorValue(left);
   } else {
     BytecodeLabel end_label;
-
-    Visit(left);
+    VisitForAccumulatorValue(left);
     builder()->JumpIfToBooleanFalse(&end_label);
-    Visit(right);
+    VisitForAccumulatorValue(right);
     builder()->Bind(&end_label);
   }
+  result_scope()->SetResultInAccumulator();
 }
 
 
 void BytecodeGenerator::VisitObjectLiteralAccessor(
     Register home_object, ObjectLiteralProperty* property, Register value_out) {
   // TODO(rmcilroy): Replace value_out with VisitForRegister();
   if (property == nullptr) {
     builder()->LoadNull().StoreAccumulatorInRegister(value_out);
   } else {
-    Visit(property->value());
+    VisitForAccumulatorValue(property->value());
     builder()->StoreAccumulatorInRegister(value_out);
     VisitSetHomeObject(value_out, home_object, property);
   }
 }
 
 
 void BytecodeGenerator::VisitSetHomeObject(Register value, Register home_object,
                                            ObjectLiteralProperty* property,
                                            int slot_number) {
   Expression* expr = property->value();
@@ skipping 58 matching lines @@
 }
 
 
 int BytecodeGenerator::feedback_index(FeedbackVectorSlot slot) const {
   return info()->feedback_vector()->GetIndex(slot);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8