[TurboProp] Fully remove successors from schedule on unreachable.

Fully remove the successor blocks when effect-control-linearization
reaches an unreachable node and is maintaining the schedule. Previously
we just updated the current_block_'s successor and removed any
unreachable predecessors from end, however if the current_block_ is not
an original block in the schedule, but a new one added due to control
flow from effect control linearization lowering, the removed successor
blocks could still be re-connected to the end block when they were
lowered. Instead, entirely remove these unreachable blocks from the
predecessor / successor chains, and have the effect-control-linearizer
avoid lowering these blocks entirely.

BUG=chromium:1076569,v8:9684

Change-Id: I4b4216019d55aef5363d88255726b85df8e7ada5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2179842Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67595}

src/compiler/effect-control-linearizer.cc

@@ -564,6 +564,12 @@ void EffectControlLinearizer::Run() {
   // TODO(rmcilroy) We should not depend on having rpo_order on schedule, and
   // instead just do our own RPO walk here.
   for (BasicBlock* block : *(schedule()->rpo_order())) {
+    if (block != schedule()->start() && block->PredecessorCount() == 0) {
+      // Block has been removed from the schedule by a preceeding unreachable
+      // node, just skip it.
+      continue;
+    }
+
     gasm()->Reset(block);
 
     BasicBlock::iterator instr = block->begin();

src/compiler/graph-assembler.cc

@@ -274,13 +274,14 @@ void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
 
   for (SuccessorInfo succ : saved_successors_) {
     BasicBlock* block = succ.block;
+    block->predecessors().erase(block->predecessors().begin() + succ.index);
     blocks.insert(block);
     worklist.push(block);
   }
+  saved_successors_.clear();
 
   // Walk through blocks until we get to the end node, then remove the path from
-  // end. Don't update successors / predecessors for intermediate nodes to keep
-  // them self-consistent, even though they are no longer part of the scheudule.
+  // end, clearing their successors / predecessors.
   // This works because the unreachable paths form self-contained control flow
   // that doesn't re-merge with reachable control flow (checked below) and
   // DeadCodeElimination::ReduceEffectPhi preventing Unreachable from going into
@@ -291,37 +292,34 @@ void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
     worklist.pop();
 
     for (BasicBlock* successor : current->successors()) {
-      if (successor == schedule_->end()) {
-        // Remove the block from end node's predecessors.
-        ZoneVector<BasicBlock*>& predecessors = successor->predecessors();
-        auto it = std::find(predecessors.begin(), predecessors.end(), current);
-        CHECK_EQ(*it, current);
-        predecessors.erase(it);
+      // Remove the block from sucessors predecessors.
+      ZoneVector<BasicBlock*>& predecessors = successor->predecessors();
+      auto it = std::find(predecessors.begin(), predecessors.end(), current);
+      DCHECK_EQ(*it, current);
+      predecessors.erase(it);
 
+      if (successor == schedule_->end()) {
+        // If we have reached the end block, remove this block's control input
+        // from the end node's control inputs.
         DCHECK_EQ(current->SuccessorCount(), 1);
-        current->ClearSuccessors();
-
-        // Remove this block's control input from the end node's control inputs.
         NodeProperties::RemoveControlFromEnd(graph_, common_,
                                              current->control_input());
       } else {
-        // Add successor to worklist if it's not already been seen.
+        // Otherwise, add successor to worklist if it's not already been seen.
         if (blocks.insert(successor).second) {
           worklist.push(successor);
         }
       }
     }
+    current->ClearSuccessors();
   }
 
 #ifdef DEBUG
   // Ensure that the set of blocks being removed from the schedule are self
-  // contained.
+  // contained, i.e., all predecessors have been removed from these blocks.
   for (BasicBlock* block : blocks) {
-    for (BasicBlock* predecessor : block->predecessors()) {
-      if (blocks.count(predecessor) == 0) {
-        CHECK_EQ(predecessor, current_block_);
-      }
-    }
+    CHECK_EQ(block->PredecessorCount(), 0);
+    CHECK_EQ(block->SuccessorCount(), 0);
   }
 #endif
 }
@@ -348,8 +346,8 @@ void GraphAssembler::BasicBlockUpdater::AddThrow(Node* node) {
     // Remove all successor blocks from the schedule.
     RemoveSuccessorsFromSchedule();
 
-    // Clear saved successors and replace with end.
-    saved_successors_.clear();
+    // Update current block's successor withend.
+    DCHECK(saved_successors_.empty());
     size_t index = schedule_->end()->predecessors().size();
     schedule_->end()->AddPredecessor(current_block_);
     saved_successors_.push_back({schedule_->end(), index});

test/unittests/compiler/effect-control-linearizer-unittest.cc

@@ -469,6 +469,64 @@ TEST_F(EffectControlLinearizerTest, UnreachableThenLoop) {
   ASSERT_THAT(schedule.end()->PredecessorAt(0), start);
 }
 
+TEST_F(EffectControlLinearizerTest, UnreachableInChangedBlockThenBranch) {
+  Schedule schedule(zone());
+
+  // Create the graph.
+  Node* truncate = graph()->NewNode(simplified()->TruncateTaggedToWord32(),
+                                    NumberConstant(1.1));
+  Node* unreachable = graph()->NewNode(common()->Unreachable(),
+                                       graph()->start(), graph()->start());
+  Node* branch =
+      graph()->NewNode(common()->Branch(), Int32Constant(0), graph()->start());
+
+  Node* if_true = graph()->NewNode(common()->IfTrue(), branch);
+  Node* true_throw = graph()->NewNode(common()->Throw(), unreachable, if_true);
+
+  Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
+  Node* false_throw =
+      graph()->NewNode(common()->Throw(), unreachable, if_false);
+
+  graph()->SetEnd(graph()->NewNode(common()->End(0)));
+
+  // Build the basic block structure.
+  BasicBlock* start = schedule.start();
+  schedule.rpo_order()->push_back(start);
+  start->set_rpo_number(0);
+
+  BasicBlock* tblock = AddBlockToSchedule(&schedule);
+  BasicBlock* fblock = AddBlockToSchedule(&schedule);
+
+  // Populate the basic blocks with nodes.
+  schedule.AddNode(start, graph()->start());
+  schedule.AddNode(start, truncate);
+  schedule.AddNode(start, unreachable);
+  schedule.AddBranch(start, branch, tblock, fblock);
+
+  schedule.AddNode(tblock, if_true);
+  schedule.AddThrow(tblock, true_throw);
+  NodeProperties::MergeControlToEnd(graph(), common(), true_throw);
+
+  schedule.AddNode(fblock, if_false);
+  schedule.AddThrow(fblock, false_throw);
+  NodeProperties::MergeControlToEnd(graph(), common(), false_throw);
+
+  ASSERT_THAT(end(), IsEnd(IsThrow(), IsThrow()));
+  ASSERT_THAT(end()->op()->ControlInputCount(), 2);
+
+  // Run the state effect linearizer, maintaining the schedule.
+  LinearizeEffectControl(
+      jsgraph(), &schedule, zone(), source_positions(), node_origins(),
+      MaskArrayIndexEnable::kDoNotMaskArrayIndex, MaintainSchedule::kMaintain);
+
+  // Start block now branches due to the lowering of TruncateTaggedToWord32, but
+  // then re-merges and the unreachable should be connected directly to end
+  // without any of the subsiquent blocks.
+  ASSERT_THAT(end(), IsEnd(IsThrow()));
+  ASSERT_THAT(end()->op()->ControlInputCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorCount(), 1);
+}
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8