[Interpreter] Enable assignments in expressions.

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 22195c1d6679c0b57568e1f50148eb501fbaeec7..a4fa91becef40b03e113c859e872ee2eba0b3eaa 100644
--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -238,8 +238,7 @@ class BytecodeGenerator::ExpressionResultScope {
 
 // Scoped class used when the result of the current expression is not
 // expected to produce a result.
-class BytecodeGenerator::EffectResultScope final
-    : public ExpressionResultScope {
+class BytecodeGenerator::EffectResultScope : public ExpressionResultScope {
  public:
   explicit EffectResultScope(BytecodeGenerator* generator)
       : ExpressionResultScope(generator, Expression::kEffect) {
@@ -297,6 +296,81 @@ class BytecodeGenerator::RegisterResultScope final
 };
 
 
+// Top-level result scope when evaluating expresions. It provides an
+// outer scope for register allocations and also ensures that
+// assignments in binary expressions produce the correct result by
+// aliasing registers to temporaries when necessary.
+class BytecodeGenerator::StatementResultScope : public EffectResultScope {

[rmcilroy, 2015/11/03 14:17:52]

nit - final

[oth, 2015/11/04 10:03:36]

Done.

+ public:
+  explicit StatementResultScope(BytecodeGenerator* generator)
+      : EffectResultScope(generator),
+        alias_mappings_(generator->zone()),
+        aliased_locals_and_parameters_(generator->zone()),
+        hazard_depth_(0) {
+    DCHECK_NULL(generator->statement_execution_result());
+    generator->set_statement_execution_result(this);
+  }
+
+  ~StatementResultScope() {
+    DCHECK_EQ(this, generator()->statement_execution_result());
+    DCHECK(hazard_depth_ == 0);
+    generator()->set_statement_execution_result(nullptr);
+  }
+
+  Register LoadFromAliasedRegister(Register reg) {

[rmcilroy, 2015/11/03 14:17:52]

Could you add a comment explaining what the return value is, also mark as
MUST_USE

[oth, 2015/11/04 10:03:36]

Done.

+    if (hazard_depth_ > 0) {
+      // Insert direct mapping if none exists as register is now of interest.
+      auto insert_result = alias_mappings_.insert(std::make_pair(reg, reg));
+      auto iterator = insert_result.first;

[rmcilroy, 2015/11/03 14:17:52]

nit - /s/iterator/previous_value

[oth, 2015/11/04 10:03:36]

Renamed to 'mapping' as it's either the value just inserted or the existing
value if it was already present.

Added a comment here.

+      return iterator->second;
+    } else {
+      return reg;
+    }
+  }
+
+  Register StoreToAliasedRegister(Register reg) {
+    if (hazard_depth_ == 0 ||
+        alias_mappings_.find(reg) == alias_mappings_.end()) {
+      return reg;
+    } else {
+      Register alias = NewRegister();
+      alias_mappings_[reg] = alias;
+      if (generator()->builder()->RegisterIsParameterOrLocal(reg)) {
+        aliased_locals_and_parameters_.insert(reg);
+      }

[rmcilroy, 2015/11/03 14:17:52]

nit - a few comments here would be helpful

[oth, 2015/11/04 10:03:36]

Done.

+      return alias;
+    }
+  }
+
+  void EnterAssignmentHazard() { hazard_depth_++; }
+
+  void LeaveAssignmentHazard() {
+    if (--hazard_depth_ == 0) {
+      StopAliasingRegisters();
+    }
+  }
+
+ private:
+  void StopAliasingRegisters() {

[rmcilroy, 2015/11/03 14:17:52]

nit - RestoreAliasedLocalsAndParameters?

[oth, 2015/11/04 10:03:36]

Done. Definitely a better name.

+    // Move temporary registers holding values for locals and
+    // parameters back into their local and parameter registers.
+    for (auto reg = aliased_locals_and_parameters_.begin();
+         reg != aliased_locals_and_parameters_.end(); reg++) {
+      auto mapping = alias_mappings_.find(*reg);
+      if (mapping != alias_mappings_.end()) {
+        builder()->MoveRegister(mapping->second, *reg);
+      }
+    }
+    alias_mappings_.clear();
+    aliased_locals_and_parameters_.clear();
+  }
+
+  ZoneMap<Register, Register> alias_mappings_;
+  ZoneSet<Register> aliased_locals_and_parameters_;
+  int hazard_depth_;
+};
+
+
 BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
     : isolate_(isolate),
       zone_(zone),
@@ -307,8 +381,7 @@ BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
       execution_control_(nullptr),
       execution_context_(nullptr),
       execution_result_(nullptr),
-      binary_expression_depth_(0),
-      binary_expression_hazard_set_(zone) {
+      statement_execution_result_(nullptr) {
   InitializeAstVisitor(isolate);
 }
 
@@ -508,8 +581,7 @@ void BytecodeGenerator::VisitDeclarations(
 
 
 void BytecodeGenerator::VisitExpressionStatement(ExpressionStatement* stmt) {
-  // TODO(rmcilroy): Replace this with a StatementResultScope when it exists.
-  EffectResultScope effect_scope(this);
+  StatementResultScope statement_result_scope(this);
   VisitForEffect(stmt->expression());
 }
 
@@ -529,7 +601,7 @@ void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
       Visit(stmt->else_statement());
     }
   } else {
-    VisitForAccumulatorValue(stmt->condition());
+    VisitCondition(stmt->condition());
     builder()->JumpIfFalse(&else_label);
     Visit(stmt->then_statement());
     if (stmt->HasElseStatement()) {
@@ -561,7 +633,7 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  EffectResultScope effect_scope(this);
+  StatementResultScope statement_result_scope(this);
   VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
@@ -626,11 +698,16 @@ void BytecodeGenerator::VisitCaseClause(CaseClause* clause) {
 }
 
 
+void BytecodeGenerator::VisitCondition(Expression* expr) {
+  StatementResultScope statement_result_scope(this);
+  VisitForAccumulatorValue(expr);
+}
+
+
 void BytecodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration execution_control(this, stmt, &loop_builder);
   BytecodeLabel body_label, condition_label, done_label;
-
   if (stmt->cond()->ToBooleanIsFalse()) {
     Visit(stmt->body());
     // Bind condition_label and done_label for processing continue and break.
@@ -644,7 +721,7 @@ void BytecodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
     if (stmt->cond()->ToBooleanIsTrue()) {
       builder()->Jump(&body_label);
     } else {
-      VisitForAccumulatorValue(stmt->cond());
+      VisitCondition(stmt->cond());
       builder()->JumpIfTrue(&body_label);
     }
     builder()->Bind(&done_label);
@@ -674,7 +751,7 @@ void BytecodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
   if (stmt->cond()->ToBooleanIsTrue()) {
     builder()->Jump(&body_label);
   } else {
-    VisitForAccumulatorValue(stmt->cond());
+    VisitCondition(stmt->cond());
     builder()->JumpIfTrue(&body_label);
   }
   builder()->Bind(&done_label);
@@ -710,7 +787,7 @@ void BytecodeGenerator::VisitForStatement(ForStatement* stmt) {
   }
   if (stmt->cond() && !stmt->cond()->ToBooleanIsTrue()) {
     builder()->Bind(&condition_label);
-    VisitForAccumulatorValue(stmt->cond());
+    VisitCondition(stmt->cond());
     builder()->JumpIfTrue(&body_label);
   } else {
     builder()->Jump(&body_label);
@@ -1200,6 +1277,9 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
       Register source(Register(variable->index()));
+      if (statement_execution_result()) {

[rmcilroy, 2015/11/03 14:17:51]

Should we not always have a statement_execution_result scope when visiting
variable loads?

[oth, 2015/11/04 10:03:36]

Right, this bit is tricky. There are places we reach here from that are not
trivial to resolve, e.g. Visiting switch statements for tag and case values. If
the StatementResultScope goes at the top-level there then return statements in
switch statements end up creating a nested StatementResultScope.

As discussed suggest we split HazardAssignmentScope into a separate class that
is instantiated in the VisitArithmeticOperation and VisitCompareOperation
methods and deals with the hazard assignment logic. The code keeps
StatementResultScope but just uses it as the outer allocation scope for places
where visits allocate registers in the parent scope.

+        source = statement_execution_result()->LoadFromAliasedRegister(source);

[rmcilroy, 2015/11/03 14:17:51]

nit - RecordLoadFrom...

[oth, 2015/11/04 10:03:36]

GetRegisterToLoadFrom?
GetRegisterToStoreTo?

+      }
       execution_result()->SetResultInRegister(source);
       break;
     }
@@ -1207,6 +1287,9 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
       // 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);
+      if (statement_execution_result()) {
+        source = statement_execution_result()->LoadFromAliasedRegister(source);
+      }
       execution_result()->SetResultInRegister(source);
       break;
     }
@@ -1270,16 +1353,22 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
     case VariableLocation::LOCAL: {
       // TODO(rmcilroy): support const mode initialization.
       Register destination(variable->index());
+      if (statement_execution_result()) {

[rmcilroy, 2015/11/03 14:17:51]

Ditto with assignments?

[oth, 2015/11/04 10:03:36]

Changed to assignment_hazard_scope, but have to check  if it exists or not
still.

+        destination =
+            statement_execution_result()->StoreToAliasedRegister(destination);

[rmcilroy, 2015/11/03 14:17:52]

nit - RecordStoreTo...

[oth, 2015/11/04 10:03:36]

GetRegisterForStore okay?

+      }
       builder()->StoreAccumulatorInRegister(destination);
-      RecordStoreToRegister(destination);
       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));
+      if (statement_execution_result()) {
+        destination =
+            statement_execution_result()->StoreToAliasedRegister(destination);
+      }
       builder()->StoreAccumulatorInRegister(destination);
-      RecordStoreToRegister(destination);
       break;
     }
     case VariableLocation::GLOBAL:
@@ -1825,38 +1914,34 @@ void BytecodeGenerator::VisitBinaryOperation(BinaryOperation* binop) {
 
 
 void BytecodeGenerator::VisitCompareOperation(CompareOperation* expr) {
-  // TODO(oth): Remove PrepareForBinaryExpression/CompleteBinaryExpression
-  // once we have StatementScope that tracks hazardous loads/stores.
-  PrepareForBinaryExpression();
+  // The evaluation of binary comparison expressions has an assignment
+  // hazard because the lhs may be a variable that evaluates to a
+  // local or parameter and the rhs may modify that, e.g. y = x + (x = 1)
+  // To get a correct result the generator treats the inner assigment
+  // as being made to a temporary x' that is spilled on exit of the
+  // assignment hazard.
+  statement_execution_result()->EnterAssignmentHazard();
   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());
-  CompleteBinaryExpression();
   execution_result()->SetResultInAccumulator();
+  statement_execution_result()->LeaveAssignmentHazard();
 }
 
 
 void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* expr) {
-  // TODO(oth): Remove PrepareForBinaryExpression/CompleteBinaryExpression
-  // once we have StatementScope that tracks hazardous loads/stores.
-  PrepareForBinaryExpression();
+  // The evaluation of binary arithmetic expressions has an assignment
+  // hazard because the lhs may be a variable that evaluates to a
+  // local or parameter and the rhs may modify that, e.g. y = x + (x = 1)
+  // To get a correct result the generator treats the inner assigment
+  // as being made to a temporary x' that is spilled on exit of the
+  // assignment hazard.
+  statement_execution_result()->EnterAssignmentHazard();

[rmcilroy, 2015/11/03 14:17:52]

Could we do this as a scoped class (AssignmentHazardScope?) instead, just to
ensure we always correctly leave the assignment hazard area even if we return in
the middle of a visit function?

[oth, 2015/11/04 10:03:36]

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.
-    // TODO(oth) LoadFromAliasedRegister call into VisitVariableLoad().
-    lhs = LoadFromAliasedRegister(lhs);
-  }
   VisitForAccumulatorValue(expr->right());
   builder()->BinaryOperation(expr->op(), lhs, language_mode_strength());
-  CompleteBinaryExpression();
   execution_result()->SetResultInAccumulator();
+  statement_execution_result()->LeaveAssignmentHazard();
 }
 
 
@@ -2076,13 +2161,6 @@ 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);
@@ -2106,35 +2184,6 @@ Register BytecodeGenerator::VisitForRegisterValue(Expression* expr) {
 }
 
 
-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.