[Interpreter] Enable assignments in expressions.

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 "src/compiler.h"
 #include "src/interpreter/control-flow-builders.h"
 #include "src/objects.h"
 #include "src/parser.h"
@@ skipping 220 matching lines @@
   ExpressionResultScope* outer_;
   TemporaryRegisterScope allocator_;
   bool result_identified_;
 
   DISALLOW_COPY_AND_ASSIGN(ExpressionResultScope);
 };
 
 
 // Scoped class used when the result of the current expression is not
 // expected to produce a result.
-class BytecodeGenerator::EffectResultScope final
-    : public ExpressionResultScope {
+class BytecodeGenerator::EffectResultScope : public ExpressionResultScope {
  public:
   explicit EffectResultScope(BytecodeGenerator* generator)
       : ExpressionResultScope(generator, Expression::kEffect) {
     set_result_identified();
   }
 
   virtual void SetResultInAccumulator() {}
   virtual void SetResultInRegister(Register reg) {}
 };
 
@@ skipping 37 matching lines @@
     set_result_identified();
   }
 
   Register ResultRegister() const { return result_register_; }
 
  private:
   Register result_register_;
 };
 
 
+// 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),
       builder_(isolate, zone),
       info_(nullptr),
       scope_(nullptr),
       globals_(0, 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);
 }
 
 
 BytecodeGenerator::~BytecodeGenerator() {}
 
 
 Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   set_info(info);
   set_scope(info->scope());
@@ skipping 179 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) {
-  // TODO(rmcilroy): Replace this with a StatementResultScope when it exists.
-  EffectResultScope effect_scope(this);
+  StatementResultScope statement_result_scope(this);
   VisitForEffect(stmt->expression());
 }
 
 
 void BytecodeGenerator::VisitEmptyStatement(EmptyStatement* stmt) {
 }
 
 
 void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
   BytecodeLabel else_label, end_label;
   if (stmt->condition()->ToBooleanIsTrue()) {
     // Generate only then block.
     Visit(stmt->then_statement());
   } else if (stmt->condition()->ToBooleanIsFalse()) {
     // Generate only else block if it exists.
     if (stmt->HasElseStatement()) {
       Visit(stmt->else_statement());
     }
   } else {
-    VisitForAccumulatorValue(stmt->condition());
+    VisitCondition(stmt->condition());
     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);
     }
     builder()->Bind(&end_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) {
-  EffectResultScope effect_scope(this);
+  StatementResultScope statement_result_scope(this);
   VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
@@ skipping 44 matching lines @@
   switch_builder.SetBreakTarget(done_label);
 }
 
 
 void BytecodeGenerator::VisitCaseClause(CaseClause* clause) {
   // Handled entirely in VisitSwitchStatement.
   UNREACHABLE();
 }
 
 
+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.
     builder()->Bind(&condition_label);
     builder()->Bind(&done_label);
   } else {
     builder()->Bind(&body_label);
     Visit(stmt->body());
 
     builder()->Bind(&condition_label);
     if (stmt->cond()->ToBooleanIsTrue()) {
       builder()->Jump(&body_label);
     } else {
-      VisitForAccumulatorValue(stmt->cond());
+      VisitCondition(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;
   if (stmt->cond()->ToBooleanIsFalse()) {
     // If the condition is false there is no need to generating the loop.
     return;
   }
 
   if (!stmt->cond()->ToBooleanIsTrue()) {
     builder()->Jump(&condition_label);
   }
   builder()->Bind(&body_label);
   Visit(stmt->body());
 
   builder()->Bind(&condition_label);
   if (stmt->cond()->ToBooleanIsTrue()) {
     builder()->Jump(&body_label);
   } else {
-    VisitForAccumulatorValue(stmt->cond());
+    VisitCondition(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) {
@@ skipping 15 matching lines @@
     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() && !stmt->cond()->ToBooleanIsTrue()) {
     builder()->Bind(&condition_label);
-    VisitForAccumulatorValue(stmt->cond());
+    VisitCondition(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 469 matching lines @@
 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(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;
     }
     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);
+      if (statement_execution_result()) {
+        source = statement_execution_result()->LoadFromAliasedRegister(source);
+      }
       execution_result()->SetResultInRegister(source);
       break;
     }
     case VariableLocation::GLOBAL:
     case VariableLocation::UNALLOCATED: {
       size_t name_index = builder()->GetConstantPoolEntry(variable->name());
       builder()->LoadGlobal(name_index, feedback_index(slot), language_mode());
       execution_result()->SetResultInAccumulator();
       break;
     }
@@ skipping 43 matching lines @@
   return register_scope.ResultRegister();
 }
 
 
 void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
                                                 FeedbackVectorSlot slot) {
   switch (variable->location()) {
     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:
     case VariableLocation::UNALLOCATED: {
       size_t name_index = builder()->GetConstantPoolEntry(variable->name());
       builder()->StoreGlobal(name_index, feedback_index(slot), language_mode());
       break;
     }
     case VariableLocation::CONTEXT: {
       // TODO(rmcilroy): support const mode initialization.
@@ skipping 525 matching lines @@
       VisitLogicalAndExpression(binop);
       break;
     default:
       VisitArithmeticExpression(binop);
       break;
   }
 }
 
 
 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();
 }
 
 
 void BytecodeGenerator::VisitSpread(Spread* expr) { UNREACHABLE(); }
 
 
 void BytecodeGenerator::VisitEmptyParentheses(EmptyParentheses* expr) {
   UNREACHABLE();
 }
 
@@ skipping 199 matching lines @@
     // Pass a SMI sentinel and let the runtime look up the empty function.
     builder()->LoadLiteral(Smi::FromInt(0));
   } else {
     DCHECK(closure_scope->is_function_scope());
     builder()->LoadAccumulatorWithRegister(Register::function_closure());
   }
   execution_result()->SetResultInAccumulator();
 }
 
 
-void BytecodeGenerator::PrepareForBinaryExpression() {
-  if (binary_expression_depth_++ == 0) {
-    binary_expression_hazard_set_.clear();
-  }
-}
-
-
 // Visits the expression |expr| and places the result in the accumulator.
 void BytecodeGenerator::VisitForAccumulatorValue(Expression* expr) {
   AccumulatorResultScope accumulator_scope(this);
   Visit(expr);
 }
 
 
 // Visits the expression |expr| and discards the result.
 void BytecodeGenerator::VisitForEffect(Expression* expr) {
   EffectResultScope effect_scope(this);
   Visit(expr);
 }
 
 
 // Visits the expression |expr| and returns the register containing
 // the expression result.
 Register BytecodeGenerator::VisitForRegisterValue(Expression* expr) {
   RegisterResultScope register_scope(this);
   Visit(expr);
   return register_scope.ResultRegister();
 }
 
 
-Register BytecodeGenerator::LoadFromAliasedRegister(Register reg) {
-  // TODO(oth): Follow on CL to load from re-map here.
-  DCHECK(builder()->RegisterIsParameterOrLocal(reg));
-  if (binary_expression_depth_ > 0) {
-    binary_expression_hazard_set_.insert(reg.index());
-  }
-  return reg;
-}
-
-
-void BytecodeGenerator::RecordStoreToRegister(Register reg) {
-  DCHECK(builder()->RegisterIsParameterOrLocal(reg));
-  if (binary_expression_depth_ > 0) {
-    // TODO(oth): a store to a register that's be loaded needs to be
-    // remapped.
-    DCHECK(binary_expression_hazard_set_.find(reg.index()) ==
-           binary_expression_hazard_set_.end());
-  }
-}
-
-
-void BytecodeGenerator::CompleteBinaryExpression() {
-  DCHECK(binary_expression_depth_ > 0);
-  binary_expression_depth_ -= 1;
-  // TODO(oth): spill remapped registers into origins.
-  // TODO(oth): make statement/top-level.
-}
-
-
 Register BytecodeGenerator::NextContextRegister() const {
   if (execution_context() == nullptr) {
     // Return the incoming function context for the outermost execution context.
     return Register::function_context();
   }
   Register previous = execution_context()->reg();
   if (previous == Register::function_context()) {
     // If the previous context was the incoming function context, then the next
     // context register is the first local context register.
     return builder_.first_context_register();
@@ skipping 15 matching lines @@
 }
 
 
 int BytecodeGenerator::feedback_index(FeedbackVectorSlot slot) const {
   return info()->feedback_vector()->GetIndex(slot);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8