[Interpreter] Ensure ToBoolean bytecodes are correctly emitted at the start of basic blocks

Existing code was assuming that 'lexical' blocks were the same as basic
blocks, therefore code which emitted jumps within a lexical block (e.g.,
logical or) would in some occassions incorrectly omit a necessary
ToBoolean.

This change removes Enter/LeaveBlock from BytecodeArrayBuilder and
instead tracks basic blocks via label bindings and jump operations. The
change also ensures we don't emit dead code at the end of a basic block,
and adds tests of the edge cases.

BUG=v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1406983010

Cr-Commit-Position: refs/heads/master@{#31741}

src/interpreter/bytecode-array-builder.cc

@@ -110,6 +110,9 @@ Handle<BytecodeArray> BytecodeArrayBuilder::ToBytecodeArray() {
 
 template <size_t N>
 void BytecodeArrayBuilder::Output(Bytecode bytecode, uint32_t(&operands)[N]) {
+  // Don't output dead code.
+  if (exit_seen_in_block_) return;
+
   DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), static_cast<int>(N));
   last_bytecode_start_ = bytecodes()->size();
   bytecodes()->push_back(Bytecodes::ToByte(bytecode));
@@ -154,6 +157,9 @@ void BytecodeArrayBuilder::Output(Bytecode bytecode, uint32_t operand0) {
 
 
 void BytecodeArrayBuilder::Output(Bytecode bytecode) {
+  // Don't output dead code.
+  if (exit_seen_in_block_) return;
+
   DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), 0);
   last_bytecode_start_ = bytecodes()->size();
   bytecodes()->push_back(Bytecodes::ToByte(bytecode));
@@ -548,17 +554,19 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::Bind(BytecodeLabel* label) {
     // Now treat as if the label will only be back referred to.
   }
   label->bind_to(bytecodes()->size());
+  LeaveBasicBlock();
   return *this;
 }
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::Bind(const BytecodeLabel& target,
                                                  BytecodeLabel* label) {
-  DCHECK_EQ(label->is_bound(), false);
-  DCHECK_EQ(target.is_bound(), true);
+  DCHECK(!label->is_bound());
+  DCHECK(target.is_bound());
   PatchJump(bytecodes()->begin() + target.offset(),
             bytecodes()->begin() + label->offset());
   label->bind_to(target.offset());
+  LeaveBasicBlock();
   return *this;
 }
 
@@ -643,6 +651,9 @@ Bytecode BytecodeArrayBuilder::GetJumpWithToBoolean(Bytecode jump_bytecode) {
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::OutputJump(Bytecode jump_bytecode,
                                                        BytecodeLabel* label) {
+  // Don't emit dead code.
+  if (exit_seen_in_block_) return *this;
+
   // Check if the value in accumulator is boolean, if not choose an
   // appropriate JumpIfToBoolean bytecode.
   if (NeedToBooleanCast()) {
@@ -674,6 +685,7 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::OutputJump(Bytecode jump_bytecode,
       UNIMPLEMENTED();
     }
   }
+  LeaveBasicBlock();
   return *this;
 }
 
@@ -737,13 +749,9 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::ForInDone(Register for_in_state) {
 }
 
 
-BytecodeArrayBuilder& BytecodeArrayBuilder::EnterBlock() { return *this; }
-
-
-BytecodeArrayBuilder& BytecodeArrayBuilder::LeaveBlock() {
+void BytecodeArrayBuilder::LeaveBasicBlock() {
   last_block_end_ = bytecodes()->size();
   exit_seen_in_block_ = false;
-  return *this;
 }
 
 

src/interpreter/bytecode-array-builder.h

@@ -192,9 +192,6 @@ class BytecodeArrayBuilder {
   BytecodeArrayBuilder& ForInNext(Register for_in_state, Register index);
   BytecodeArrayBuilder& ForInDone(Register for_in_state);
 
-  BytecodeArrayBuilder& EnterBlock();
-  BytecodeArrayBuilder& LeaveBlock();
-
   // Accessors
   Zone* zone() const { return zone_; }
 
@@ -238,6 +235,7 @@ class BytecodeArrayBuilder {
   void PatchJump(const ZoneVector<uint8_t>::iterator& jump_target,
                  ZoneVector<uint8_t>::iterator jump_location);
 
+  void LeaveBasicBlock();
   void EnsureReturn();
 
   bool OperandIsValid(Bytecode bytecode, int operand_index,

src/interpreter/bytecode-generator.cc

@@ -374,7 +374,6 @@ void BytecodeGenerator::MakeBytecodeBody() {
 
 
 void BytecodeGenerator::VisitBlock(Block* stmt) {
-  builder()->EnterBlock();
   if (stmt->scope() == NULL) {
     // Visit statements in the same scope, no declarations.
     VisitStatements(stmt->statements());
@@ -390,7 +389,6 @@ void BytecodeGenerator::VisitBlock(Block* stmt) {
       VisitStatements(stmt->statements());
     }
   }
-  builder()->LeaveBlock();
 }
 
 
@@ -782,7 +780,6 @@ void BytecodeGenerator::VisitForInStatement(ForInStatement* stmt) {
   ControlScopeForIteration control_scope(this, stmt, &loop_builder);
 
   // Prepare the state for executing ForIn.
-  builder()->EnterBlock();
   VisitForAccumulatorValue(stmt->subject());
   loop_builder.BreakIfUndefined();
   loop_builder.BreakIfNull();
@@ -828,8 +825,8 @@ void BytecodeGenerator::VisitForInStatement(ForInStatement* stmt) {
       .CountOperation(Token::Value::ADD, language_mode_strength())
       .Jump(&condition_label);
 
-  // End of loop
-  builder()->Bind(&break_label).LeaveBlock();
+  // End of the loop.
+  builder()->Bind(&break_label);
 
   loop_builder.SetBreakTarget(break_label);
   loop_builder.SetContinueTarget(continue_label);

test/cctest/interpreter/test-interpreter.cc

@@ -1527,12 +1527,11 @@ TEST(InterpreterUnaryNot) {
     builder.set_locals_count(0);
     builder.set_context_count(0);
     builder.set_parameter_count(0);
-    builder.EnterBlock();
     builder.LoadFalse();
     for (size_t j = 0; j < i; j++) {
       builder.LogicalNot();
     }
-    builder.LeaveBlock().Return();
+    builder.Return();
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray();
     InterpreterTester tester(handles.main_isolate(), bytecode_array);
     auto callable = tester.GetCallable<>();
@@ -1591,10 +1590,9 @@ TEST(InterpreterToBoolean) {
     builder.set_locals_count(0);
     builder.set_context_count(0);
     builder.set_parameter_count(0);
-    builder.EnterBlock();
     LoadAny(&builder, factory, object_type_tuples[i].first);
     builder.CastAccumulatorToBoolean();
-    builder.LeaveBlock().Return();
+    builder.Return();
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray();
     InterpreterTester tester(handles.main_isolate(), bytecode_array);
     auto callable = tester.GetCallable<>();
@@ -1629,10 +1627,9 @@ TEST(InterpreterUnaryNotNonBoolean) {
     builder.set_locals_count(0);
     builder.set_context_count(0);
     builder.set_parameter_count(0);
-    builder.EnterBlock();
     LoadAny(&builder, factory, object_type_tuples[i].first);
     builder.LogicalNot();
-    builder.LeaveBlock().Return();
+    builder.Return();
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray();
     InterpreterTester tester(handles.main_isolate(), bytecode_array);
     auto callable = tester.GetCallable<>();
@@ -1665,10 +1662,9 @@ TEST(InterpreterTypeOf) {
     builder.set_locals_count(0);
     builder.set_context_count(0);
     builder.set_parameter_count(0);
-    builder.EnterBlock();
     LoadAny(&builder, factory, object_type_tuples[i].first);
     builder.TypeOf();
-    builder.LeaveBlock().Return();
+    builder.Return();
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray();
     InterpreterTester tester(handles.main_isolate(), bytecode_array);
     auto callable = tester.GetCallable<>();

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

@@ -185,9 +185,12 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
       .BinaryOperation(Token::Value::ADD, reg, Strength::WEAK)
       .JumpIfFalse(&start);
 
-  builder.EnterBlock()
-      .Throw()
-      .LeaveBlock();
+  // Emit throw in it's own basic block so that the rest of the code isn't
+  // omitted due to being dead.
+  BytecodeLabel after_throw;
+  builder.Jump(&after_throw)
+    .Throw()
+    .Bind(&after_throw);
 
   builder.ForInPrepare(reg).ForInDone(reg).ForInNext(reg, reg);
 
@@ -483,19 +486,19 @@ TEST_F(BytecodeArrayBuilderTest, BackwardJumps) {
   BytecodeLabel label0, label1, label2, label3, label4;
   builder.Bind(&label0)
       .Jump(&label0)
-      .CompareOperation(Token::Value::EQ, reg, Strength::WEAK)
       .Bind(&label1)
-      .JumpIfTrue(&label1)
       .CompareOperation(Token::Value::EQ, reg, Strength::WEAK)
+      .JumpIfTrue(&label1)
       .Bind(&label2)
+      .CompareOperation(Token::Value::EQ, reg, Strength::WEAK)
       .JumpIfFalse(&label2)
-      .BinaryOperation(Token::Value::ADD, reg, Strength::WEAK)
       .Bind(&label3)
-      .JumpIfTrue(&label3)
       .BinaryOperation(Token::Value::ADD, reg, Strength::WEAK)
+      .JumpIfTrue(&label3)
       .Bind(&label4)
+      .BinaryOperation(Token::Value::ADD, reg, Strength::WEAK)
       .JumpIfFalse(&label4);
-  for (int i = 0; i < 64; i++) {
+  for (int i = 0; i < 63; i++) {
     builder.Jump(&label4);
   }
   builder.BinaryOperation(Token::Value::ADD, reg, Strength::WEAK)
@@ -517,26 +520,26 @@ TEST_F(BytecodeArrayBuilderTest, BackwardJumps) {
   // Ignore compare operation.
   iterator.Advance();
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kJumpIfTrue);
-  CHECK_EQ(iterator.GetImmediateOperand(0), 0);
+  CHECK_EQ(iterator.GetImmediateOperand(0), -2);
   iterator.Advance();
   // Ignore compare operation.
   iterator.Advance();
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kJumpIfFalse);
-  CHECK_EQ(iterator.GetImmediateOperand(0), 0);
+  CHECK_EQ(iterator.GetImmediateOperand(0), -2);
   iterator.Advance();
   // Ignore binary operation.
   iterator.Advance();
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kJumpIfToBooleanTrue);
-  CHECK_EQ(iterator.GetImmediateOperand(0), 0);
+  CHECK_EQ(iterator.GetImmediateOperand(0), -2);
   iterator.Advance();
   // Ignore binary operation.
   iterator.Advance();
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kJumpIfToBooleanFalse);
-  CHECK_EQ(iterator.GetImmediateOperand(0), 0);
+  CHECK_EQ(iterator.GetImmediateOperand(0), -2);
   iterator.Advance();
-  for (int i = 0; i < 64; i++) {
+  for (int i = 0; i < 63; i++) {
     CHECK_EQ(iterator.current_bytecode(), Bytecode::kJump);
-    CHECK_EQ(iterator.GetImmediateOperand(0), -i * 2 - 2);
+    CHECK_EQ(iterator.GetImmediateOperand(0), -i * 2 - 4);
     iterator.Advance();
   }
   // Ignore binary operation.
@@ -561,7 +564,7 @@ TEST_F(BytecodeArrayBuilderTest, BackwardJumps) {
   CHECK_EQ(Smi::cast(*iterator.GetConstantForIndexOperand(0))->value(), -156);
   iterator.Advance();
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kJumpConstant);
-  CHECK_EQ(Smi::cast(*iterator.GetConstantForIndexOperand(0))->value(), -162);
+  CHECK_EQ(Smi::cast(*iterator.GetConstantForIndexOperand(0))->value(), -160);
   iterator.Advance();
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kReturn);
   iterator.Advance();
@@ -638,8 +641,8 @@ TEST_F(BytecodeArrayBuilderTest, ToBoolean) {
   builder.set_locals_count(0);
   builder.set_context_count(0);
 
-  // Check ToBoolean emitted at start of block.
-  builder.EnterBlock().CastAccumulatorToBoolean();
+  // Check ToBoolean emitted at start of a basic block.
+  builder.CastAccumulatorToBoolean();
 
   // Check ToBoolean emitted preceding bytecode is non-boolean.
   builder.LoadNull().CastAccumulatorToBoolean();
@@ -647,12 +650,18 @@ TEST_F(BytecodeArrayBuilderTest, ToBoolean) {
   // Check ToBoolean omitted if preceding bytecode is boolean.
   builder.LoadFalse().CastAccumulatorToBoolean();
 
-  // Check ToBoolean emitted if it is at the start of the next block.
+  // Check ToBoolean emitted if it is at the start of a basic block caused by a
+  // bound label.
+  BytecodeLabel label;
   builder.LoadFalse()
-      .LeaveBlock()
-      .EnterBlock()
-      .CastAccumulatorToBoolean()
-      .LeaveBlock();
+      .Bind(&label)
+      .CastAccumulatorToBoolean();
+
+  // Check ToBoolean emitted if it is at the start of a basic block caused by a
+  // jump.
+  builder.LoadFalse()
+      .JumpIfTrue(&label)
+      .CastAccumulatorToBoolean();
 
   builder.Return();
 
@@ -674,6 +683,13 @@ TEST_F(BytecodeArrayBuilderTest, ToBoolean) {
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kToBoolean);
   iterator.Advance();
 
+  CHECK_EQ(iterator.current_bytecode(), Bytecode::kLdaFalse);
+  iterator.Advance();
+  CHECK_EQ(iterator.current_bytecode(), Bytecode::kJumpIfTrue);
+  iterator.Advance();
+  CHECK_EQ(iterator.current_bytecode(), Bytecode::kToBoolean);
+  iterator.Advance();
+
   CHECK_EQ(iterator.current_bytecode(), Bytecode::kReturn);
   iterator.Advance();
   CHECK(iterator.done());