[interpreter] Minor clean-up of BytecodeSourceInfo.

Add explicit state in BytecodeSourceInfo to simplify checks for
validity and whether a statement or expression position.

Remove BytecodeSourceInfo::Update which inherited rules for updating
source position information during bytecode building.

BUG=v8:4280
LOG=N

Review-Url: https://codereview.chromium.org/2048203002
Cr-Commit-Position: refs/heads/master@{#37136}

src/interpreter/bytecode-array-builder.cc

@@ -99,7 +99,7 @@ void BytecodeArrayBuilder::AttachSourceInfo(BytecodeNode* node) {
     // information if it is used.
     if (latest_source_info_.is_statement() ||
         ExpressionPositionIsNeeded(node->bytecode())) {
-      node->source_info() = latest_source_info_;
+      node->source_info().Clone(latest_source_info_);
       latest_source_info_.set_invalid();
     }
   }
@@ -440,9 +440,17 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::JumpIfNotHole(
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::StackCheck(int position) {
   if (position != RelocInfo::kNoPosition) {
-    // We need to attach a non-breakable source position to a stack check,
-    // so we simply add it as expression position.
-    latest_source_info_ = {position, false};
+    // We need to attach a non-breakable source position to a stack
+    // check, so we simply add it as expression position. There can be
+    // a prior statement position from constructs like:
+    //
+    //    do var x;  while (false);
+    //
+    // A Nop could be inserted for empty statements, but since no code
+    // is associated with these positions, instead we force the stack
+    // check's expression position which eliminates the empty
+    // statement's position.
+    latest_source_info_.ForceExpressionPosition(position);
   }
   Output(Bytecode::kStackCheck);
   return *this;
@@ -620,31 +628,26 @@ size_t BytecodeArrayBuilder::GetConstantPoolEntry(Handle<Object> object) {
 
 void BytecodeArrayBuilder::SetReturnPosition() {
   if (return_position_ == RelocInfo::kNoPosition) return;
-  latest_source_info_.Update({return_position_, true});
+  latest_source_info_.MakeStatementPosition(return_position_);
 }
 
 void BytecodeArrayBuilder::SetStatementPosition(Statement* stmt) {
   if (stmt->position() == RelocInfo::kNoPosition) return;
-  latest_source_info_.Update({stmt->position(), true});
+  latest_source_info_.MakeStatementPosition(stmt->position());
 }
 
 void BytecodeArrayBuilder::SetExpressionPosition(Expression* expr) {
   if (expr->position() == RelocInfo::kNoPosition) return;
-  if (latest_source_info_.is_expression()) {
+  if (!latest_source_info_.is_statement()) {
     // Ensure the current expression position is overwritten with the
     // latest value.
-    //
-    // TODO(oth): Clean-up BytecodeSourceInfo to have three states and
-    // simplify the update logic, taking care to ensure position
-    // information is not lost.
-    latest_source_info_.set_invalid();
+    latest_source_info_.MakeExpressionPosition(expr->position());
   }
-  latest_source_info_.Update({expr->position(), false});
 }
 
 void BytecodeArrayBuilder::SetExpressionAsStatementPosition(Expression* expr) {
   if (expr->position() == RelocInfo::kNoPosition) return;
-  latest_source_info_.Update({expr->position(), true});
+  latest_source_info_.MakeStatementPosition(expr->position());
 }
 
 bool BytecodeArrayBuilder::TemporaryRegisterIsLive(Register reg) const {

src/interpreter/bytecode-peephole-optimizer.cc

@@ -297,7 +297,10 @@ BytecodeNode* BytecodePeepholeOptimizer::Optimize(BytecodeNode* current) {
   }
 
   if (CanElideLast(current) && CanElideLastBasedOnSourcePosition(current)) {
-    current->source_info().Update(last_.source_info());
+    if (last_.source_info().is_valid()) {
+      // Current can not be valid per CanElideLastBasedOnSourcePosition().
+      current->source_info().Clone(last_.source_info());
+    }
     InvalidateLast();
     return current;
   }

src/interpreter/bytecode-pipeline.cc

@@ -11,26 +11,6 @@ namespace v8 {
 namespace internal {
 namespace interpreter {
 
-void BytecodeSourceInfo::Update(const BytecodeSourceInfo& entry) {
-  if (!entry.is_valid()) return;
-
-  if (!is_valid() || (entry.is_statement() && !is_statement()) ||
-      (entry.is_statement() && is_statement() &&
-       entry.source_position() > source_position())) {
-    // Position is updated if any of the following conditions are met:
-    //   (1) there is no existing position.
-    //   (2) the incoming position is a statement and the current position
-    //       is an expression.
-    //   (3) the existing position is a statement and the incoming
-    //       statement has a later source position.
-    // Condition 3 is needed for the first statement in a function which
-    // may end up with later statement positions being added during bytecode
-    // generation.
-    source_position_ = entry.source_position_;
-    is_statement_ = entry.is_statement_;
-  }
-}
-
 BytecodeNode::BytecodeNode(Bytecode bytecode) {
   DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), 0);
   bytecode_ = bytecode;

src/interpreter/bytecode-pipeline.h

@@ -54,36 +54,86 @@ class BytecodeSourceInfo final {
  public:
   static const int kUninitializedPosition = -1;
 
-  BytecodeSourceInfo(int position = kUninitializedPosition,
-                     bool is_statement = false)
-      : source_position_(position), is_statement_(is_statement) {}
+  BytecodeSourceInfo()
+      : position_type_(PositionType::kNone),
+        source_position_(kUninitializedPosition) {}
+
+  BytecodeSourceInfo(int source_position, bool is_statement)
+      : position_type_(is_statement ? PositionType::kStatement
+                                    : PositionType::kExpression),
+        source_position_(source_position) {
+    DCHECK_GE(source_position, 0);
+  }
+
+  // Makes instance into a statement position.
+  void MakeStatementPosition(int source_position) {
+    // Statement positions can be replaced by other statement
+    // positions. For example , "for (x = 0; x < 3; ++x) 7;" has a
+    // statement position associated with 7 but no bytecode associated
+    // with it. Then Next is emitted after the body and has
+    // statement position and overrides the existing one.
+    position_type_ = PositionType::kStatement;
+    source_position_ = source_position;
+  }
+
+  // Makes instance into an expression position. Instance should not
+  // be a statement position otherwise it could be lost and impair the
+  // debugging experience.
+  void MakeExpressionPosition(int source_position) {
+    DCHECK(!is_statement());
+    position_type_ = PositionType::kExpression;
+    source_position_ = source_position;
+  }
+
+  // Forces an instance into an expression position.
+  void ForceExpressionPosition(int source_position) {
+    position_type_ = PositionType::kExpression;
+    source_position_ = source_position;
+  }
 
-  // Combine later source info with current.
-  void Update(const BytecodeSourceInfo& entry);
+  // Clones a source position. The current instance is expected to be
+  // invalid.
+  void Clone(const BytecodeSourceInfo& other) {
+    DCHECK(!is_valid());
+    position_type_ = other.position_type_;
+    source_position_ = other.source_position_;
+  }
 
   int source_position() const {
     DCHECK(is_valid());
     return source_position_;
   }
 
-  bool is_statement() const { return is_valid() && is_statement_; }
-  bool is_expression() const { return is_valid() && !is_statement_; }
+  bool is_statement() const {
+    return position_type_ == PositionType::kStatement;
+  }
+  bool is_expression() const {
+    return position_type_ == PositionType::kExpression;
+  }
 
-  bool is_valid() const { return source_position_ != kUninitializedPosition; }
-  void set_invalid() { source_position_ = kUninitializedPosition; }
+  bool is_valid() const { return position_type_ != PositionType::kNone; }
+  void set_invalid() {
+    position_type_ = PositionType::kNone;
+    source_position_ = kUninitializedPosition;
+  }
 
   bool operator==(const BytecodeSourceInfo& other) const {
-    return source_position_ == other.source_position_ &&
-           is_statement_ == other.is_statement_;
+    return position_type_ == other.position_type_ &&
+           source_position_ == other.source_position_;
   }
+
   bool operator!=(const BytecodeSourceInfo& other) const {
-    return source_position_ != other.source_position_ ||
-           is_statement_ != other.is_statement_;
+    return position_type_ != other.position_type_ ||
+           source_position_ != other.source_position_;
   }
 
  private:
+  enum class PositionType : uint8_t { kNone, kExpression, kStatement };
+
+  PositionType position_type_;
   int source_position_;
-  bool is_statement_;
+
+  DISALLOW_COPY_AND_ASSIGN(BytecodeSourceInfo);
 };
 
 // A container for a generated bytecode, it's operands, and source information.

src/interpreter/bytecode-register-optimizer.cc

@@ -312,7 +312,9 @@ void BytecodeRegisterOptimizer::WriteToNextStage(BytecodeNode* node) const {
 
 void BytecodeRegisterOptimizer::WriteToNextStage(
     BytecodeNode* node, const BytecodeSourceInfo& source_info) const {
-  node->source_info().Update(source_info);
+  if (source_info.is_valid()) {
+    node->source_info().Clone(source_info);
+  }
   next_stage_->Write(node);
 }
 
@@ -414,8 +416,9 @@ void BytecodeRegisterOptimizer::RegisterTransfer(
 
 void BytecodeRegisterOptimizer::EmitNopForSourceInfo(
     const BytecodeSourceInfo& source_info) const {
+  DCHECK(source_info.is_valid());
   BytecodeNode nop(Bytecode::kNop);
-  nop.source_info().Update(source_info);
+  nop.source_info().Clone(source_info);
   WriteToNextStage(&nop);
 }
 

test/unittests/interpreter/bytecode-array-writer-unittest.cc

@@ -60,7 +60,7 @@ class BytecodeArrayWriterUnittest : public TestWithIsolateAndZone {
 void BytecodeArrayWriterUnittest::Write(BytecodeNode* node,
                                         const BytecodeSourceInfo& info) {
   if (info.is_valid()) {
-    node->source_info().Update(info);
+    node->source_info().Clone(info);
   }
   writer()->Write(node);
 }
@@ -104,7 +104,7 @@ void BytecodeArrayWriterUnittest::WriteJump(Bytecode bytecode,
                                             const BytecodeSourceInfo& info) {
   BytecodeNode node(bytecode, 0);
   if (info.is_valid()) {
-    node.source_info().Update(info);
+    node.source_info().Clone(info);
   }
   writer()->WriteJump(&node, label);
 }

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

@@ -103,7 +103,7 @@ TEST_F(BytecodePeepholeOptimizerTest, ElideEmptyNop) {
 
 TEST_F(BytecodePeepholeOptimizerTest, ElideExpressionNop) {
   BytecodeNode nop(Bytecode::kNop);
-  nop.source_info().Update({3, false});
+  nop.source_info().MakeExpressionPosition(3);
   optimizer()->Write(&nop);
   BytecodeNode add(Bytecode::kAdd, Register(0).ToOperand());
   optimizer()->Write(&add);
@@ -114,10 +114,10 @@ TEST_F(BytecodePeepholeOptimizerTest, ElideExpressionNop) {
 
 TEST_F(BytecodePeepholeOptimizerTest, KeepStatementNop) {
   BytecodeNode nop(Bytecode::kNop);
-  nop.source_info().Update({3, true});
+  nop.source_info().MakeStatementPosition(3);
   optimizer()->Write(&nop);
   BytecodeNode add(Bytecode::kAdd, Register(0).ToOperand());
-  add.source_info().Update({3, false});
+  add.source_info().MakeExpressionPosition(3);
   optimizer()->Write(&add);
   Flush();
   CHECK_EQ(write_count(), 2);
@@ -210,7 +210,7 @@ TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRx) {
 TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRxStatement) {
   BytecodeNode first(Bytecode::kStar, Register(0).ToOperand());
   BytecodeNode second(Bytecode::kLdar, Register(0).ToOperand());
-  second.source_info().Update({0, true});
+  second.source_info().MakeStatementPosition(0);
   optimizer()->Write(&first);
   CHECK_EQ(write_count(), 0);
   optimizer()->Write(&second);
@@ -227,7 +227,7 @@ TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRxStatementStarRy) {
   BytecodeNode first(Bytecode::kStar, Register(0).ToOperand());
   BytecodeNode second(Bytecode::kLdar, Register(0).ToOperand());
   BytecodeNode third(Bytecode::kStar, Register(3).ToOperand());
-  second.source_info().Update({0, true});
+  second.source_info().MakeStatementPosition(0);
   optimizer()->Write(&first);
   CHECK_EQ(write_count(), 0);
   optimizer()->Write(&second);
@@ -237,8 +237,6 @@ TEST_F(BytecodePeepholeOptimizerTest, StarRxLdarRxStatementStarRy) {
   CHECK_EQ(write_count(), 1);
   Flush();
   CHECK_EQ(write_count(), 2);
-  // Source position should move |second| to |third| when |second| is elided.
-  third.source_info().Update(second.source_info());
   CHECK_EQ(last_written(), third);
 }
 
@@ -325,7 +323,7 @@ TEST_F(BytecodePeepholeOptimizerTest, LdaTrueLdaFalse) {
 
 TEST_F(BytecodePeepholeOptimizerTest, LdaTrueStatementLdaFalse) {
   BytecodeNode first(Bytecode::kLdaTrue);
-  first.source_info().Update({3, false});
+  first.source_info().MakeExpressionPosition(3);
   BytecodeNode second(Bytecode::kLdaFalse);
   optimizer()->Write(&first);
   CHECK_EQ(write_count(), 0);
@@ -333,8 +331,9 @@ TEST_F(BytecodePeepholeOptimizerTest, LdaTrueStatementLdaFalse) {
   CHECK_EQ(write_count(), 0);
   Flush();
   CHECK_EQ(write_count(), 1);
-  second.source_info().Update(first.source_info());
   CHECK_EQ(last_written(), second);
+  CHECK(second.source_info().is_expression());
+  CHECK_EQ(second.source_info().source_position(), 3);
 }
 
 TEST_F(BytecodePeepholeOptimizerTest, NopStackCheck) {
@@ -351,7 +350,7 @@ TEST_F(BytecodePeepholeOptimizerTest, NopStackCheck) {
 
 TEST_F(BytecodePeepholeOptimizerTest, NopStatementStackCheck) {
   BytecodeNode first(Bytecode::kNop);
-  first.source_info().Update({3, false});
+  first.source_info().MakeExpressionPosition(3);
   BytecodeNode second(Bytecode::kStackCheck);
   optimizer()->Write(&first);
   CHECK_EQ(write_count(), 0);
@@ -359,7 +358,8 @@ TEST_F(BytecodePeepholeOptimizerTest, NopStatementStackCheck) {
   CHECK_EQ(write_count(), 0);
   Flush();
   CHECK_EQ(write_count(), 1);
-  second.source_info().Update(first.source_info());
+  second.source_info().MakeExpressionPosition(
+      first.source_info().source_position());
   CHECK_EQ(last_written(), second);
 }
 

test/unittests/interpreter/bytecode-pipeline-unittest.cc

@@ -24,7 +24,7 @@ TEST(BytecodeSourceInfo, Operations) {
   CHECK_EQ(x.is_statement(), false);
   CHECK_EQ(x.is_valid(), false);
 
-  x.Update({1, true});
+  x.MakeStatementPosition(1);
   BytecodeSourceInfo y(1, true);
   CHECK(x == y);
   CHECK(!(x != y));
@@ -33,20 +33,20 @@ TEST(BytecodeSourceInfo, Operations) {
   CHECK(!(x == y));
   CHECK(x != y);
 
-  y.Update({2, false});
+  y.MakeStatementPosition(1);
   CHECK_EQ(y.source_position(), 1);
   CHECK_EQ(y.is_statement(), true);
 
-  y.Update({2, true});
+  y.MakeStatementPosition(2);
   CHECK_EQ(y.source_position(), 2);
   CHECK_EQ(y.is_statement(), true);
 
   y.set_invalid();
-  y.Update({3, false});
+  y.MakeExpressionPosition(3);
   CHECK_EQ(y.source_position(), 3);
   CHECK_EQ(y.is_statement(), false);
 
-  y.Update({3, true});
+  y.MakeStatementPosition(3);
   CHECK_EQ(y.source_position(), 3);
   CHECK_EQ(y.is_statement(), true);
 }
@@ -122,11 +122,11 @@ TEST_F(BytecodeNodeTest, EqualityWithSourceInfo) {
   uint32_t operands[] = {0x71, 0xa5, 0x5a, 0xfc};
   BytecodeNode node(Bytecode::kForInNext, operands[0], operands[1], operands[2],
                     operands[3]);
-  node.source_info().Update({3, true});
+  node.source_info().MakeStatementPosition(3);
   CHECK_EQ(node, node);
   BytecodeNode other(Bytecode::kForInNext, operands[0], operands[1],
                      operands[2], operands[3]);
-  other.source_info().Update({3, true});
+  other.source_info().MakeStatementPosition(3);
   CHECK_EQ(node, other);
 }
 
@@ -134,7 +134,7 @@ TEST_F(BytecodeNodeTest, NoEqualityWithDifferentSourceInfo) {
   uint32_t operands[] = {0x71, 0xa5, 0x5a, 0xfc};
   BytecodeNode node(Bytecode::kForInNext, operands[0], operands[1], operands[2],
                     operands[3]);
-  node.source_info().Update({3, true});
+  node.source_info().MakeStatementPosition(3);
   BytecodeNode other(Bytecode::kForInNext, operands[0], operands[1],
                      operands[2], operands[3]);
   CHECK_NE(node, other);
@@ -154,7 +154,8 @@ TEST_F(BytecodeNodeTest, SetBytecode0) {
   BytecodeNode node(Bytecode::kForInNext, operands[0], operands[1], operands[2],
                     operands[3]);
   BytecodeSourceInfo source_info(77, false);
-  node.source_info().Update(source_info);
+  node.source_info().Clone(source_info);
+  CHECK_EQ(node.source_info(), source_info);
 
   BytecodeNode clone;
   clone.Clone(&node);
@@ -169,7 +170,7 @@ TEST_F(BytecodeNodeTest, SetBytecode1) {
   BytecodeNode node(Bytecode::kForInNext, operands[0], operands[1], operands[2],
                     operands[3]);
   BytecodeSourceInfo source_info(77, false);
-  node.source_info().Update(source_info);
+  node.source_info().Clone(source_info);
 
   BytecodeNode clone;
   clone.Clone(&node);

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

@@ -75,7 +75,7 @@ TEST_F(BytecodeRegisterOptimizerTest, WriteNop) {
 TEST_F(BytecodeRegisterOptimizerTest, WriteNopExpression) {
   Initialize(1, 1);
   BytecodeNode node(Bytecode::kNop);
-  node.source_info().Update({3, false});
+  node.source_info().MakeExpressionPosition(3);
   optimizer()->Write(&node);
   CHECK_EQ(write_count(), 1);
   CHECK_EQ(node, last_written());
@@ -84,7 +84,7 @@ TEST_F(BytecodeRegisterOptimizerTest, WriteNopExpression) {
 TEST_F(BytecodeRegisterOptimizerTest, WriteNopStatement) {
   Initialize(1, 1);
   BytecodeNode node(Bytecode::kNop);
-  node.source_info().Update({3, true});
+  node.source_info().MakeStatementPosition(3);
   optimizer()->Write(&node);
   CHECK_EQ(write_count(), 1);
   CHECK_EQ(node, last_written());