[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 {

[rmcilroy, 2015/11/05 16:34:51]

Make final again

[oth, 2015/11/12 11:32:15]

Done.

  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_;
 };
 
 
+// Scope providing protection against potentially conflicting patterns
+// of variable loads and stores in binary expressions.
+class BytecodeGenerator::AssignmentHazardScope final BASE_EMBEDDED {
+ public:
+  explicit AssignmentHazardScope(BytecodeGenerator* generator)
+      : alias_mappings_(generator->zone()),
+        aliased_locals_and_parameters_(generator->zone()),
+        register_scope_(generator->builder()) {
+    if (generator->assignment_hazard_scope() == nullptr) {
+      // Set self as the assignment_hazard_scope if one doesn't
+      // exist. Only the outermost participates in register remapping.
+      generator_ = generator;
+      generator->set_assignment_hazard_scope(this);

[rmcilroy, 2015/11/05 16:34:51]

Rather than creating a map and set for each AssignmentHazardScope, could
AssignmentHazardScope just be a wrapper around another class which does the
actual register remapping, and create / set that class if the
AssignmentHazardScope is the outer-most one. E.g.:

explicit AssignmentHazardScope(BytecodeGenerator* generator) {
  if (generator->aliased_local_and_parameter_helper() == nullptr) {
    helper_ = new AliasedLocalAndParameterHelper(generator);
    generator->set_aliased_local_and_parameter_helper(helper_);
  }
}

~AssignmentHazardScope() {
  if (helper_) {
    helper_->RestoreAliasedLocalsAndParameters();
    generator->set_aliased_local_and_parameter_helper(nullptr);
  }
}

And move the rest of the code into AliasedLocalAndParameterHelper, WDYT?

Actually, thinking about it further, the AliasedLocalAndParameterHelper could
just be a permanent member of generator and AssignmentHazardScope could just
call EnableAssignmentHazard() and RestoreAliasedLocalsAndParameters() in the
constructor / destructor, which would avoid the "if (assignment_hazard_scope())
{" checks in  VisitVariableLoad / VisitVariableAssignment).

[oth, 2015/11/12 11:32:15]

Okay, the helper class an inner class of BytecodeGenerator. The state was in
BytecodeGenerator before the detour into StatementExpessionResult. The test
in load/stores happens in the GetRegisterForLoad/Store so it looks cleaner.

+    } else {
+      generator_ = nullptr;
+    }
+  }
+
+  ~AssignmentHazardScope() {
+    if (is_outermost_scope()) {
+      RestoreAliasedLocalsAndParameters();
+      generator()->set_assignment_hazard_scope(nullptr);
+    }
+  }
+
+  // Returns a register that a load instruction should use when
+  // loading from |reg|. This allows an alias for a modified version
+  // of |reg| to be used within a hazard regions.
+  MUST_USE_RESULT Register GetRegisterForLoad(Register reg) {
+    DCHECK(is_outermost_scope());
+    // A load from |reg| is to be issued. The register is placed in
+    // the mappings table initially mapping to itself. Future stores
+    // will update the mapping with temporaries thus preserving the
+    // original register's value.
+    //
+    // NB This insert only updates the table if no mapping exists
+    // already (std::map::insert semantics).
+    auto insert_result =
+        alias_mappings_.insert(std::make_pair(reg.index(), reg.index()));
+    auto mapping = insert_result.first;
+    // Return the current alias for reg.
+    return Register(mapping->second);
+  }
+
+  // Returns a register that a store instruction should use when
+  // loading from |reg|. This allows an alias for a modified version
+  // of |reg| to be used within hazard regions.
+  MUST_USE_RESULT Register GetRegisterForStore(Register reg) {
+    DCHECK(is_outermost_scope());
+    if (alias_mappings_.find(reg.index()) == alias_mappings_.end()) {
+      // If not in a hazard region or a load for this register has not
+      // occurred no mapping is necessary.
+      return reg;
+    } else {
+      // Storing to a register with 1 or more loads issued. The
+      // register is mapped to a temporary alias so we don't overwrite
+      // the lhs value, e.g.  y = x + (x = 1); has a register for x on
+      // the lhs and needs a new register x' for the upcoming store on
+      // the rhs as the original x is an input to the add operation.
+      Register alias = register_scope_.NewRegister();
+      alias_mappings_[reg.index()] = alias.index();
+      if (generator()->builder()->RegisterIsParameterOrLocal(reg)) {
+        // Keep track of registers that need to be restored on exit
+        // from the assignment hazard region.
+        aliased_locals_and_parameters_.insert(reg.index());
+      }
+      return alias;
+    }
+  }
+
+ private:
+  void RestoreAliasedLocalsAndParameters() {
+    // 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(Register(mapping->second), Register(*reg));
+      }
+    }
+    alias_mappings_.clear();
+    aliased_locals_and_parameters_.clear();
+  }
+
+  BytecodeArrayBuilder* builder() const { return generator()->builder(); }
+  BytecodeGenerator* generator() const { return generator_; }
+  bool is_outermost_scope() const { return generator_ != nullptr; }
+
+  BytecodeGenerator* generator_;
+  ZoneMap<int, int> alias_mappings_;
+  ZoneSet<int> aliased_locals_and_parameters_;
+  TemporaryRegisterScope register_scope_;
+
+  DISALLOW_COPY_AND_ASSIGN(AssignmentHazardScope);
+};
+
+
 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) {
+      assignment_hazard_scope_(nullptr) {
   InitializeAstVisitor(isolate);
 }
 
 
 BytecodeGenerator::~BytecodeGenerator() {}
 
 
 Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   set_info(info);
   set_scope(info->scope());
@@ skipping 177 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);
+  EffectResultScope statement_result_scope(this);
   VisitForEffect(stmt->expression());
 }
 
 
 void BytecodeGenerator::VisitEmptyStatement(EmptyStatement* stmt) {
 }
 
 
 void BytecodeGenerator::VisitIfStatement(IfStatement* stmt) {
   BytecodeLabel else_label, end_label;
@@ skipping 31 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);
+  EffectResultScope statement_result_scope(this);
   VisitForAccumulatorValue(stmt->expression());
   builder()->Return();
 }
 
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
   UNIMPLEMENTED();
 }
 
 
@@ skipping 48 matching lines @@
 void BytecodeGenerator::VisitCaseClause(CaseClause* clause) {
   // Handled entirely in VisitSwitchStatement.
   UNREACHABLE();
 }
 
 
 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);
@@ skipping 115 matching lines @@
       break;
     }
     case NAMED_SUPER_PROPERTY:
     case KEYED_SUPER_PROPERTY:
       UNIMPLEMENTED();
   }
 }
 
 
 void BytecodeGenerator::VisitForInStatement(ForInStatement* stmt) {
-  // TODO(oth): For now we need a parent scope for paths that end up
-  // in VisitLiteral which can allocate in the parent scope. A future
-  // CL in preparation will add a StatementResultScope that will
-  // remove the need for this EffectResultScope.
-  EffectResultScope result_scope(this);
+  EffectResultScope statement_result_scope(this);
 
   if (stmt->subject()->IsNullLiteral() ||
       stmt->subject()->IsUndefinedLiteral(isolate())) {
     // ForIn generates lots of code, skip if it wouldn't produce any effects.
     return;
   }
 
   LoopBuilder loop_builder(builder());
   ControlScopeForIteration control_scope(this, stmt, &loop_builder);
 
@@ skipping 409 matching lines @@
   VisitVariableLoad(proxy->var(), proxy->VariableFeedbackSlot());
 }
 
 
 void BytecodeGenerator::VisitVariableLoad(Variable* variable,
                                           FeedbackVectorSlot slot,
                                           TypeofMode typeof_mode) {
   switch (variable->location()) {
     case VariableLocation::LOCAL: {
       Register source(Register(variable->index()));
+      if (assignment_hazard_scope()) {
+        source = assignment_hazard_scope()->GetRegisterForLoad(source);
+      }
       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 (assignment_hazard_scope()) {
+        source = assignment_hazard_scope()->GetRegisterForLoad(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(),
                             typeof_mode);
       execution_result()->SetResultInAccumulator();
       break;
@@ skipping 44 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 (assignment_hazard_scope()) {
+        destination =
+            assignment_hazard_scope()->GetRegisterForStore(destination);
+      }
       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 (assignment_hazard_scope()) {
+        destination =
+            assignment_hazard_scope()->GetRegisterForStore(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 526 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.
+  AssignmentHazardScope assignment_hazard_scope(this);
+
   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();
 }
 
 
 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.
+  AssignmentHazardScope assignment_hazard_scope(this);
+
   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();
 }
 
 
 void BytecodeGenerator::VisitSpread(Spread* expr) { UNREACHABLE(); }
 
 
 void BytecodeGenerator::VisitEmptyParentheses(EmptyParentheses* expr) {
   UNREACHABLE();
 }
@@ skipping 200 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