[Interpreter] Remove nop elision from peephole and be smarter about emitting nops.

Rather than doing nop elision in the peephole optimizer, be smarter about
emitting nops for elided register transfers in the bytecode optimizer.

BUG=v8:6194

Change-Id: Ib1a7168a0d143e4f2da7c6d43080998793c30822
Reviewed-on: https://chromium-review.googlesource.com/468929
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44479}

src/interpreter/bytecode-array-builder.cc

@@ -17,6 +17,25 @@ namespace v8 {
 namespace internal {
 namespace interpreter {
 
+class RegisterTransferWriter final
+    : public NON_EXPORTED_BASE(BytecodeRegisterOptimizer::BytecodeWriter),
+      public NON_EXPORTED_BASE(ZoneObject) {
+ public:
+  RegisterTransferWriter(BytecodeArrayBuilder* builder) : builder_(builder) {}
+  ~RegisterTransferWriter() override {}
+
+  void EmitLdar(Register input) override { builder_->OutputLdarRaw(input); }
+
+  void EmitStar(Register output) override { builder_->OutputStarRaw(output); }
+
+  void EmitMov(Register input, Register output) override {
+    builder_->OutputMovRaw(input, output);
+  }
+
+ private:
+  BytecodeArrayBuilder* builder_;
+};
+
 BytecodeArrayBuilder::BytecodeArrayBuilder(
     Isolate* isolate, Zone* zone, int parameter_count, int context_count,
     int locals_count, FunctionLiteral* literal,
@@ -50,7 +69,7 @@ BytecodeArrayBuilder::BytecodeArrayBuilder(
   if (FLAG_ignition_reo) {
     register_optimizer_ = new (zone) BytecodeRegisterOptimizer(
         zone, &register_allocator_, fixed_register_count(), parameter_count,
-        pipeline_);
+        new (zone) RegisterTransferWriter(this));
   }
 
   return_position_ = literal ? literal->return_position() : kNoSourcePosition;
@@ -119,6 +138,59 @@ BytecodeSourceInfo BytecodeArrayBuilder::CurrentSourcePosition(
   return source_position;
 }
 
+void BytecodeArrayBuilder::SetDeferredSourceInfo(
+    BytecodeSourceInfo source_info) {
+  if (!source_info.is_valid()) return;
+  if (deferred_source_info_.is_valid()) {
+    // Emit any previous deferred source info now as a nop.
+    BytecodeNode node = BytecodeNode::Nop(deferred_source_info_);
+    pipeline()->Write(&node);
+  }
+  deferred_source_info_ = source_info;
+}
+
+void BytecodeArrayBuilder::AttachOrEmitDeferredSourceInfo(BytecodeNode* node) {
+  if (!deferred_source_info_.is_valid()) return;
+
+  if (!node->source_info().is_valid()) {
+    node->set_source_info(deferred_source_info_);
+  } else {
+    BytecodeNode node = BytecodeNode::Nop(deferred_source_info_);
+    pipeline()->Write(&node);
+  }
+  deferred_source_info_.set_invalid();
+}
+
+void BytecodeArrayBuilder::Write(BytecodeNode* node) {
+  AttachOrEmitDeferredSourceInfo(node);
+  pipeline()->Write(node);
+}
+
+void BytecodeArrayBuilder::WriteJump(BytecodeNode* node, BytecodeLabel* label) {
+  AttachOrEmitDeferredSourceInfo(node);
+  pipeline()->WriteJump(node, label);
+}
+
+void BytecodeArrayBuilder::OutputLdarRaw(Register reg) {
+  uint32_t operand = static_cast<uint32_t>(reg.ToOperand());
+  BytecodeNode node(BytecodeNode::Ldar(BytecodeSourceInfo(), operand));
+  Write(&node);
+}
+
+void BytecodeArrayBuilder::OutputStarRaw(Register reg) {
+  uint32_t operand = static_cast<uint32_t>(reg.ToOperand());
+  BytecodeNode node(BytecodeNode::Star(BytecodeSourceInfo(), operand));
+  Write(&node);
+}
+
+void BytecodeArrayBuilder::OutputMovRaw(Register src, Register dest) {
+  uint32_t operand0 = static_cast<uint32_t>(src.ToOperand());
+  uint32_t operand1 = static_cast<uint32_t>(dest.ToOperand());
+  BytecodeNode node(
+      BytecodeNode::Mov(BytecodeSourceInfo(), operand0, operand1));
+  Write(&node);
+}
+
 namespace {
 
 template <OperandTypeInfo type_info>
@@ -257,7 +329,7 @@ class BytecodeNodeBuilder {
         BytecodeNodeBuilder<Bytecode::k##name, __VA_ARGS__>::Make<       \
             Operands...>(this, CurrentSourcePosition(Bytecode::k##name), \
                          operands...));                                  \
-    pipeline()->Write(&node);                                            \
+    Write(&node);                                                        \
   }                                                                      \
                                                                          \
   template <typename... Operands>                                        \
@@ -268,7 +340,7 @@ class BytecodeNodeBuilder {
         BytecodeNodeBuilder<Bytecode::k##name, __VA_ARGS__>::Make<       \
             Operands...>(this, CurrentSourcePosition(Bytecode::k##name), \
                          operands...));                                  \
-    pipeline()->WriteJump(&node, label);                                 \
+    WriteJump(&node, label);                                             \
     LeaveBasicBlock();                                                   \
   }
 BYTECODE_LIST(DEFINE_BYTECODE_OUTPUT)
@@ -566,7 +638,10 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::LoadFalse() {
 BytecodeArrayBuilder& BytecodeArrayBuilder::LoadAccumulatorWithRegister(
     Register reg) {
   if (register_optimizer_) {
-    register_optimizer_->DoLdar(reg, CurrentSourcePosition(Bytecode::kLdar));
+    // Defer source info so that if we elide the bytecode transfer, we attach
+    // the source info to a subsequent bytecode or to a nop.
+    SetDeferredSourceInfo(CurrentSourcePosition(Bytecode::kLdar));
+    register_optimizer_->DoLdar(reg);
   } else {
     OutputLdar(reg);
   }
@@ -576,7 +651,10 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::LoadAccumulatorWithRegister(
 BytecodeArrayBuilder& BytecodeArrayBuilder::StoreAccumulatorInRegister(
     Register reg) {
   if (register_optimizer_) {
-    register_optimizer_->DoStar(reg, CurrentSourcePosition(Bytecode::kStar));
+    // Defer source info so that if we elide the bytecode transfer, we attach
+    // the source info to a subsequent bytecode or to a nop.
+    SetDeferredSourceInfo(CurrentSourcePosition(Bytecode::kStar));
+    register_optimizer_->DoStar(reg);
   } else {
     OutputStar(reg);
   }
@@ -587,7 +665,10 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::MoveRegister(Register from,
                                                          Register to) {
   DCHECK(from != to);
   if (register_optimizer_) {
-    register_optimizer_->DoMov(from, to, CurrentSourcePosition(Bytecode::kMov));
+    // Defer source info so that if we elide the bytecode transfer, we attach
+    // the source info to a subsequent bytecode or to a nop.
+    SetDeferredSourceInfo(CurrentSourcePosition(Bytecode::kMov));
+    register_optimizer_->DoMov(from, to);
   } else {
     OutputMov(from, to);
   }

src/interpreter/bytecode-array-builder.h

@@ -434,6 +434,12 @@ class V8_EXPORT_PRIVATE BytecodeArrayBuilder final
   uint32_t GetInputRegisterListOperand(RegisterList reg_list);
   uint32_t GetOutputRegisterListOperand(RegisterList reg_list);
 
+  // Outputs raw register transfer bytecodes without going through the register
+  // optimizer.
+  void OutputLdarRaw(Register reg);
+  void OutputStarRaw(Register reg);
+  void OutputMovRaw(Register src, Register dest);
+
   // Accessors
   BytecodeRegisterAllocator* register_allocator() {
     return &register_allocator_;
@@ -470,6 +476,17 @@ class V8_EXPORT_PRIVATE BytecodeArrayBuilder final
   // Set position for return.
   void SetReturnPosition();
 
+  // Sets a deferred source info which should be emitted before any future
+  // source info (either attached to a following bytecode or as a nop).
+  void SetDeferredSourceInfo(BytecodeSourceInfo source_info);
+  // Either attach deferred source info to node, or emit it as a nop bytecode
+  // if node already have valid source info.
+  void AttachOrEmitDeferredSourceInfo(BytecodeNode* node);
+
+  // Write bytecode to bytecode array.
+  void Write(BytecodeNode* node);
+  void WriteJump(BytecodeNode* node, BytecodeLabel* label);
+
   // Not implemented as the illegal bytecode is used inside internally
   // to indicate a bytecode field is not valid or an error has occured
   // during bytecode generation.
@@ -509,6 +526,7 @@ class V8_EXPORT_PRIVATE BytecodeArrayBuilder final
   BytecodePipelineStage* pipeline_;
   BytecodeRegisterOptimizer* register_optimizer_;
   BytecodeSourceInfo latest_source_info_;
+  BytecodeSourceInfo deferred_source_info_;
 
   static int const kNoFeedbackSlot = 0;
 

src/interpreter/bytecode-register-optimizer.cc

@@ -194,13 +194,13 @@ BytecodeRegisterOptimizer::RegisterInfo::GetEquivalent() {
 BytecodeRegisterOptimizer::BytecodeRegisterOptimizer(
     Zone* zone, BytecodeRegisterAllocator* register_allocator,
     int fixed_registers_count, int parameter_count,
-    BytecodePipelineStage* next_stage)
+    BytecodeWriter* bytecode_writer)
     : accumulator_(Register::virtual_accumulator()),
       temporary_base_(fixed_registers_count),
       max_register_index_(fixed_registers_count - 1),
       register_info_table_(zone),
       equivalence_id_(0),
-      next_stage_(next_stage),
+      bytecode_writer_(bytecode_writer),
       flush_required_(false),
       zone_(zone) {
   register_allocator->set_observer(this);
@@ -257,25 +257,17 @@ void BytecodeRegisterOptimizer::Flush() {
 }
 
 void BytecodeRegisterOptimizer::OutputRegisterTransfer(
-    RegisterInfo* input_info, RegisterInfo* output_info,
-    BytecodeSourceInfo source_info) {
+    RegisterInfo* input_info, RegisterInfo* output_info) {
   Register input = input_info->register_value();
   Register output = output_info->register_value();
   DCHECK_NE(input.index(), output.index());
 
   if (input == accumulator_) {
-    uint32_t operand = static_cast<uint32_t>(output.ToOperand());
-    BytecodeNode node = BytecodeNode::Star(source_info, operand);
-    next_stage_->Write(&node);
+    bytecode_writer_->EmitStar(output);
   } else if (output == accumulator_) {
-    uint32_t operand = static_cast<uint32_t>(input.ToOperand());
-    BytecodeNode node = BytecodeNode::Ldar(source_info, operand);
-    next_stage_->Write(&node);
+    bytecode_writer_->EmitLdar(input);
   } else {
-    uint32_t operand0 = static_cast<uint32_t>(input.ToOperand());
-    uint32_t operand1 = static_cast<uint32_t>(output.ToOperand());
-    BytecodeNode node = BytecodeNode::Mov(source_info, operand0, operand1);
-    next_stage_->Write(&node);
+    bytecode_writer_->EmitMov(input, output);
   }
   if (output != accumulator_) {
     max_register_index_ = std::max(max_register_index_, output.index());
@@ -328,9 +320,8 @@ void BytecodeRegisterOptimizer::AddToEquivalenceSet(
   flush_required_ = true;
 }
 
-void BytecodeRegisterOptimizer::RegisterTransfer(
-    RegisterInfo* input_info, RegisterInfo* output_info,
-    BytecodeSourceInfo source_info) {
+void BytecodeRegisterOptimizer::RegisterTransfer(RegisterInfo* input_info,
+                                                 RegisterInfo* output_info) {
   // Materialize an alternate in the equivalence set that
   // |output_info| is leaving.
   if (output_info->materialized()) {
@@ -348,10 +339,7 @@ void BytecodeRegisterOptimizer::RegisterTransfer(
     // Force store to be emitted when register is observable.
     output_info->set_materialized(false);
     RegisterInfo* materialized_info = input_info->GetMaterializedEquivalent();
-    OutputRegisterTransfer(materialized_info, output_info, source_info);
-  } else if (source_info.is_valid()) {
-    // Emit a placeholder nop to maintain source position info.
-    EmitNopForSourceInfo(source_info);
+    OutputRegisterTransfer(materialized_info, output_info);
   }
 
   bool input_is_observable = RegisterIsObservable(input_info->register_value());
@@ -362,13 +350,6 @@ void BytecodeRegisterOptimizer::RegisterTransfer(
   }
 }
 
-void BytecodeRegisterOptimizer::EmitNopForSourceInfo(
-    BytecodeSourceInfo source_info) const {
-  DCHECK(source_info.is_valid());
-  BytecodeNode nop = BytecodeNode::Nop(source_info);
-  next_stage_->Write(&nop);
-}
-
 void BytecodeRegisterOptimizer::PrepareOutputRegister(Register reg) {
   RegisterInfo* reg_info = GetRegisterInfo(reg);
   if (reg_info->materialized()) {

src/interpreter/bytecode-register-optimizer.h

@@ -7,7 +7,7 @@
 
 #include "src/base/compiler-specific.h"
 #include "src/globals.h"
-#include "src/interpreter/bytecode-pipeline.h"
+#include "src/interpreter/bytecode-register-allocator.h"
 
 namespace v8 {
 namespace internal {
@@ -21,25 +21,39 @@ class V8_EXPORT_PRIVATE BytecodeRegisterOptimizer final
     : public NON_EXPORTED_BASE(BytecodeRegisterAllocator::Observer),
       public NON_EXPORTED_BASE(ZoneObject) {
  public:
+  class BytecodeWriter {
+   public:
+    BytecodeWriter() {}
+    virtual ~BytecodeWriter() {}
+
+    // Called to emit a register transfer bytecode.
+    virtual void EmitLdar(Register input) = 0;
+    virtual void EmitStar(Register output) = 0;
+    virtual void EmitMov(Register input, Register output) = 0;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(BytecodeWriter);
+  };
+
   BytecodeRegisterOptimizer(Zone* zone,
                             BytecodeRegisterAllocator* register_allocator,
                             int fixed_registers_count, int parameter_count,
-                            BytecodePipelineStage* next_stage);
+                            BytecodeWriter* bytecode_writer);
   virtual ~BytecodeRegisterOptimizer() {}
 
   // Perform explicit register transfer operations.
-  void DoLdar(Register input, BytecodeSourceInfo source_info) {
+  void DoLdar(Register input) {
     RegisterInfo* input_info = GetRegisterInfo(input);
-    RegisterTransfer(input_info, accumulator_info_, source_info);
+    RegisterTransfer(input_info, accumulator_info_);
   }
-  void DoStar(Register output, BytecodeSourceInfo source_info) {
+  void DoStar(Register output) {
     RegisterInfo* output_info = GetRegisterInfo(output);
-    RegisterTransfer(accumulator_info_, output_info, source_info);
+    RegisterTransfer(accumulator_info_, output_info);
   }
-  void DoMov(Register input, Register output, BytecodeSourceInfo source_info) {
+  void DoMov(Register input, Register output) {
     RegisterInfo* input_info = GetRegisterInfo(input);
     RegisterInfo* output_info = GetRegisterInfo(output);
-    RegisterTransfer(input_info, output_info, source_info);
+    RegisterTransfer(input_info, output_info);
   }
 
   // Materialize all live registers and flush equivalence sets.
@@ -98,20 +112,11 @@ class V8_EXPORT_PRIVATE BytecodeRegisterOptimizer final
   void RegisterListAllocateEvent(RegisterList reg_list) override;
   void RegisterListFreeEvent(RegisterList reg) override;
 
-  // Update internal state for register transfer from |input| to
-  // |output| using |source_info| as source position information if
-  // any bytecodes are emitted due to transfer.
-  void RegisterTransfer(RegisterInfo* input, RegisterInfo* output,
-                        BytecodeSourceInfo source_info);
+  // Update internal state for register transfer from |input| to |output|
+  void RegisterTransfer(RegisterInfo* input, RegisterInfo* output);
 
   // Emit a register transfer bytecode from |input| to |output|.
-  void OutputRegisterTransfer(
-      RegisterInfo* input, RegisterInfo* output,
-      BytecodeSourceInfo source_info = BytecodeSourceInfo());
-
-  // Emits a Nop to preserve source position information in the
-  // bytecode pipeline.
-  void EmitNopForSourceInfo(BytecodeSourceInfo source_info) const;
+  void OutputRegisterTransfer(RegisterInfo* input, RegisterInfo* output);
 
   void CreateMaterializedEquivalent(RegisterInfo* info);
   RegisterInfo* GetMaterializedEquivalent(RegisterInfo* info);
@@ -181,7 +186,7 @@ class V8_EXPORT_PRIVATE BytecodeRegisterOptimizer final
   // Counter for equivalence sets identifiers.
   int equivalence_id_;
 
-  BytecodePipelineStage* next_stage_;
+  BytecodeWriter* bytecode_writer_;
   bool flush_required_;
   Zone* zone_;
 

src/interpreter/mkpeephole.cc

@@ -79,16 +79,6 @@ const char* PeepholeActionTableWriter::kNamespaceElements[] = {"v8", "internal",
 // static
 PeepholeActionAndData PeepholeActionTableWriter::LookupActionAndData(
     Bytecode last, Bytecode current) {
-  // Nop are placeholders for holding source position information and can be
-  // elided if there is no source information.
-  if (last == Bytecode::kNop) {
-    if (Bytecodes::IsJump(current)) {
-      return {PeepholeAction::kElideLastBeforeJumpAction, Bytecode::kIllegal};
-    } else {
-      return {PeepholeAction::kElideLastAction, Bytecode::kIllegal};
-    }
-  }
-
   // The accumulator is invisible to the debugger. If there is a sequence
   // of consecutive accumulator loads (that don't have side effects) then
   // only the final load is potentially visible.

test/unittests/interpreter/bytecode-peephole-optimizer-unittest.cc

@@ -92,42 +92,6 @@ TEST_F(BytecodePeepholeOptimizerTest, FlushOnBind) {
   CHECK_EQ(add, last_written());
 }
 
-// Nop elimination tests.
-
-TEST_F(BytecodePeepholeOptimizerTest, ElideEmptyNop) {
-  BytecodeNode nop(Bytecode::kNop);
-  optimizer()->Write(&nop);
-  BytecodeNode add(Bytecode::kAdd, Register(0).ToOperand(), 1);
-  optimizer()->Write(&add);
-  Flush();
-  CHECK_EQ(write_count(), 1);
-  CHECK_EQ(add, last_written());
-}
-
-TEST_F(BytecodePeepholeOptimizerTest, ElideExpressionNop) {
-  BytecodeSourceInfo source_info(3, false);
-  BytecodeNode nop(Bytecode::kNop, source_info);
-  optimizer()->Write(&nop);
-  BytecodeNode add(Bytecode::kAdd, Register(0).ToOperand(), 1);
-  optimizer()->Write(&add);
-  Flush();
-  CHECK_EQ(write_count(), 1);
-  CHECK_EQ(add, last_written());
-}
-
-TEST_F(BytecodePeepholeOptimizerTest, KeepStatementNop) {
-  BytecodeSourceInfo source_info_statement(3, true);
-  BytecodeNode nop(Bytecode::kNop, source_info_statement);
-  optimizer()->Write(&nop);
-  BytecodeSourceInfo source_info_expression(3, false);
-  BytecodeNode add(Bytecode::kAdd, Register(0).ToOperand(), 1,
-                   source_info_expression);
-  optimizer()->Write(&add);
-  Flush();
-  CHECK_EQ(write_count(), 2);
-  CHECK_EQ(add, last_written());
-}
-
 // Tests covering BytecodePeepholeOptimizer::CanElideCurrent().
 
 TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRy) {
@@ -170,24 +134,6 @@ TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRxStatement) {
   CHECK_EQ(last_written().source_info(), source_info);
 }
 
-TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRxStatementStarRy) {
-  BytecodeLabel label;
-  BytecodeNode first(Bytecode::kStar, Register(0).ToOperand());
-  BytecodeSourceInfo source_info(0, true);
-  BytecodeNode second(Bytecode::kLdar, Register(0).ToOperand(), source_info);
-  BytecodeNode third(Bytecode::kStar, Register(3).ToOperand());
-  optimizer()->Write(&first);
-  CHECK_EQ(write_count(), 0);
-  optimizer()->Write(&second);
-  CHECK_EQ(write_count(), 1);
-  CHECK_EQ(last_written(), first);
-  optimizer()->Write(&third);
-  CHECK_EQ(write_count(), 1);
-  Flush();
-  CHECK_EQ(write_count(), 2);
-  CHECK_EQ(last_written(), third);
-}
-
 // Tests covering BytecodePeepholeOptimizer::CanElideLast().
 
 TEST_F(BytecodePeepholeOptimizerTest, LdaTrueLdaFalse) {
@@ -217,32 +163,6 @@ TEST_F(BytecodePeepholeOptimizerTest, LdaTrueStatementLdaFalse) {
   CHECK_EQ(second.source_info().source_position(), 3);
 }
 
-TEST_F(BytecodePeepholeOptimizerTest, NopStackCheck) {
-  BytecodeNode first(Bytecode::kNop);
-  BytecodeNode second(Bytecode::kStackCheck);
-  optimizer()->Write(&first);
-  CHECK_EQ(write_count(), 0);
-  optimizer()->Write(&second);
-  CHECK_EQ(write_count(), 0);
-  Flush();
-  CHECK_EQ(write_count(), 1);
-  CHECK_EQ(last_written(), second);
-}
-
-TEST_F(BytecodePeepholeOptimizerTest, NopStatementStackCheck) {
-  BytecodeSourceInfo source_info(3, true);
-  BytecodeNode first(Bytecode::kNop, source_info);
-  BytecodeNode second(Bytecode::kStackCheck);
-  optimizer()->Write(&first);
-  CHECK_EQ(write_count(), 0);
-  optimizer()->Write(&second);
-  CHECK_EQ(write_count(), 0);
-  Flush();
-  CHECK_EQ(write_count(), 1);
-  BytecodeNode expected(Bytecode::kStackCheck, source_info);
-  CHECK_EQ(last_written(), expected);
-}
-
 // Tests covering BytecodePeepholeOptimizer::UpdateLastAndCurrentBytecodes().
 
 }  // namespace interpreter