[TurboProp] Remove unreachable successor basic blocks from schedule.

Effect-control-linearizer will update a basic block to connect it
directly to the end node if it has an Unreachable node. Usually the
block would already have been connected directly to end (via a Throw
node) already, however in some cases it can be connected indirectly
(via a branch, where both end in a throw node).

If this happens, and the Effect-control-linearizer is maintaining the
schedule (e.g., for TurboProp), it will cause the end block to have
unreachable predecessor blocks, which can cause issues with the
register allocator.

To fix this, have the BasicBlockUpdater remove all successor blocks
from the schedule, when they become Unreachable. Also add some tests
to cover this in effect-control-linearizer-unittests.

BUG=v8:10332,v8:9684

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

src/compiler/graph-assembler.cc

@@ -19,7 +19,8 @@ namespace compiler {
 
 class GraphAssembler::BasicBlockUpdater {
  public:
-  BasicBlockUpdater(Schedule* schedule, Graph* graph, Zone* temp_zone);
+  BasicBlockUpdater(Schedule* schedule, Graph* graph,
+                    CommonOperatorBuilder* common, Zone* temp_zone);
 
   Node* AddNode(Node* node);
   Node* AddNode(Node* node, BasicBlock* to);
@@ -48,6 +49,7 @@ class GraphAssembler::BasicBlockUpdater {
   bool IsOriginalNode(Node* node);
   void UpdateSuccessors(BasicBlock* block);
   void SetBlockDeferredFromPredecessors();
+  void RemoveSuccessorsFromSchedule();
   void CopyForChange();
 
   Zone* temp_zone_;
@@ -64,6 +66,7 @@ class GraphAssembler::BasicBlockUpdater {
 
   Schedule* schedule_;
   Graph* graph_;
+  CommonOperatorBuilder* common_;
 
   // The nodes in the original block if we are in 'changed' state. Retained to
   // avoid invalidating iterators that are iterating over the original nodes of
@@ -85,14 +88,15 @@ class GraphAssembler::BasicBlockUpdater {
   State state_;
 };
 
-GraphAssembler::BasicBlockUpdater::BasicBlockUpdater(Schedule* schedule,
-                                                     Graph* graph,
-                                                     Zone* temp_zone)
+GraphAssembler::BasicBlockUpdater::BasicBlockUpdater(
+    Schedule* schedule, Graph* graph, CommonOperatorBuilder* common,
+    Zone* temp_zone)
     : temp_zone_(temp_zone),
       current_block_(nullptr),
       original_block_(nullptr),
       schedule_(schedule),
       graph_(graph),
+      common_(common),
       saved_nodes_(schedule->zone()),
       saved_successors_(schedule->zone()),
       original_control_(BasicBlock::kNone),
@@ -264,11 +268,68 @@ void GraphAssembler::BasicBlockUpdater::AddGoto(BasicBlock* from,
   current_block_ = nullptr;
 }
 
+void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
+  ZoneSet<BasicBlock*> blocks(temp_zone());
+  ZoneQueue<BasicBlock*> worklist(temp_zone());
+
+  for (SuccessorInfo succ : saved_successors_) {
+    BasicBlock* block = succ.block;
+    blocks.insert(block);
+    worklist.push(block);
+  }
+
+  // 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.
+  // 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
+  // an effect-phi. We would need to extend this if we need the ability to mark
+  // control flow as unreachable later in the pipeline.
+  while (!worklist.empty()) {
+    BasicBlock* current = worklist.front();
+    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);
+
+        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.
+        if (blocks.insert(successor).second) {
+          worklist.push(successor);
+        }
+      }
+    }
+  }
+
+#ifdef DEBUG
+  // Ensure that the set of blocks being removed from the schedule are self
+  // contained.
+  for (BasicBlock* block : blocks) {
+    for (BasicBlock* predecessor : block->predecessors()) {
+      if (blocks.count(predecessor) == 0) {
+        CHECK_EQ(predecessor, current_block_);
+      }
+    }
+  }
+#endif
+}
+
 void GraphAssembler::BasicBlockUpdater::AddThrow(Node* node) {
   if (state_ == kUnchanged) {
     CopyForChange();
   }
-  schedule_->AddThrow(current_block_, node);
 
   // Clear original successors and replace the block's original control and
   // control input to the throw, since this block is now connected directly to
@@ -280,10 +341,19 @@ void GraphAssembler::BasicBlockUpdater::AddThrow(Node* node) {
   original_control_input_ = node;
   original_control_ = BasicBlock::kThrow;
 
-  for (SuccessorInfo succ : saved_successors_) {
-    succ.block->RemovePredecessor(succ.index);
+  bool already_connected_to_end =
+      saved_successors_.size() == 1 &&
+      saved_successors_[0].block == schedule_->end();
+  if (!already_connected_to_end) {
+    // Remove all successor blocks from the schedule.
+    RemoveSuccessorsFromSchedule();
+
+    // Clear saved successors and replace with end.
+    saved_successors_.clear();
+    size_t index = schedule_->end()->predecessors().size();
+    schedule_->end()->AddPredecessor(current_block_);
+    saved_successors_.push_back({schedule_->end(), index});
   }
-  saved_successors_.clear();
 }
 
 void GraphAssembler::BasicBlockUpdater::UpdateSuccessors(BasicBlock* block) {
@@ -341,9 +411,10 @@ GraphAssembler::GraphAssembler(MachineGraph* mcgraph, Zone* zone,
       mcgraph_(mcgraph),
       effect_(nullptr),
       control_(nullptr),
-      block_updater_(schedule != nullptr ? new BasicBlockUpdater(
-                                               schedule, mcgraph->graph(), zone)
-                                         : nullptr),
+      block_updater_(schedule != nullptr
+                         ? new BasicBlockUpdater(schedule, mcgraph->graph(),
+                                                 mcgraph->common(), zone)
+                         : nullptr),
       loop_headers_(zone),
       mark_loop_exits_(mark_loop_exits) {}
 

src/compiler/node-properties.cc

@@ -158,6 +158,21 @@ void NodeProperties::MergeControlToEnd(Graph* graph,
   graph->end()->set_op(common->End(graph->end()->InputCount()));
 }
 
+void NodeProperties::RemoveControlFromEnd(Graph* graph,
+                                          CommonOperatorBuilder* common,
+                                          Node* node) {
+  int index_to_remove = -1;
+  for (int i = 0; i < graph->end()->op()->ControlInputCount(); i++) {
+    int index = NodeProperties::FirstControlIndex(graph->end()) + i;
+    if (graph->end()->InputAt(index) == node) {
+      index_to_remove = index;
+      break;
+    }
+  }
+  CHECK_NE(-1, index_to_remove);
+  graph->end()->RemoveInput(index_to_remove);
+  graph->end()->set_op(common->End(graph->end()->InputCount()));
+}
 
 // static
 void NodeProperties::ReplaceUses(Node* node, Node* value, Node* effect,

src/compiler/node-properties.h

@@ -140,6 +140,11 @@ class V8_EXPORT_PRIVATE NodeProperties final {
   static void MergeControlToEnd(Graph* graph, CommonOperatorBuilder* common,
                                 Node* node);
 
+  // Removes the control node {node} from the end of the graph, reducing the
+  // existing merge node's input count.
+  static void RemoveControlFromEnd(Graph* graph, CommonOperatorBuilder* common,
+                                   Node* node);
+
   // Replace all uses of {node} with the given replacement nodes. All occurring
   // use kinds need to be replaced, {nullptr} is only valid if a use kind is
   // guaranteed not to exist.

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

@@ -292,6 +292,183 @@ TEST_F(EffectControlLinearizerTest, CloneBranch) {
                                             IsBranch(cond2, control2)))))));
 }
 
+TEST_F(EffectControlLinearizerTest, UnreachableThenBranch) {
+  Schedule schedule(zone());
+
+  // Create the graph.
+  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, 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);
+
+  // Initial block with 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.start()->SuccessorCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorAt(0), start);
+}
+
+TEST_F(EffectControlLinearizerTest, UnreachableThenDiamond) {
+  Schedule schedule(zone());
+
+  // Create the graph.
+  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* if_false = graph()->NewNode(common()->IfFalse(), branch);
+
+  Node* merge = graph()->NewNode(common()->Merge(2), if_true, if_false);
+  Node* throw_node = 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);
+  BasicBlock* mblock = AddBlockToSchedule(&schedule);
+
+  // Populate the basic blocks with nodes.
+  schedule.AddNode(start, graph()->start());
+  schedule.AddNode(start, unreachable);
+  schedule.AddBranch(start, branch, tblock, fblock);
+
+  schedule.AddNode(tblock, if_true);
+  schedule.AddGoto(tblock, mblock);
+
+  schedule.AddNode(fblock, if_false);
+  schedule.AddGoto(fblock, mblock);
+
+  schedule.AddNode(mblock, merge);
+  schedule.AddThrow(mblock, throw_node);
+  NodeProperties::MergeControlToEnd(graph(), common(), throw_node);
+
+  ASSERT_THAT(end(), IsEnd(IsThrow()));
+  ASSERT_THAT(end()->op()->ControlInputCount(), 1);
+
+  // Run the state effect linearizer, maintaining the schedule.
+  LinearizeEffectControl(
+      jsgraph(), &schedule, zone(), source_positions(), node_origins(),
+      MaskArrayIndexEnable::kDoNotMaskArrayIndex, MaintainSchedule::kMaintain);
+
+  // Initial block with 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.start()->SuccessorCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorAt(0), start);
+}
+
+TEST_F(EffectControlLinearizerTest, UnreachableThenLoop) {
+  Schedule schedule(zone());
+
+  // Create the graph.
+  Node* unreachable = graph()->NewNode(common()->Unreachable(),
+                                       graph()->start(), graph()->start());
+  Node* loop = graph()->NewNode(common()->Loop(1), graph()->start());
+
+  Node* cond = Int32Constant(0);
+  Node* branch = graph()->NewNode(common()->Branch(), cond, loop);
+
+  Node* if_true = graph()->NewNode(common()->IfTrue(), branch);
+
+  Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
+
+  loop->AppendInput(zone(), if_false);
+  NodeProperties::ChangeOp(loop, common()->Loop(2));
+
+  Node* throw_node = 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* lblock = AddBlockToSchedule(&schedule);
+  BasicBlock* fblock = AddBlockToSchedule(&schedule);
+  BasicBlock* tblock = AddBlockToSchedule(&schedule);
+
+  // Populate the basic blocks with nodes.
+  schedule.AddNode(start, graph()->start());
+  schedule.AddNode(start, unreachable);
+  schedule.AddGoto(start, lblock);
+
+  schedule.AddNode(lblock, loop);
+  schedule.AddNode(lblock, cond);
+  schedule.AddBranch(lblock, branch, tblock, fblock);
+
+  schedule.AddNode(fblock, if_false);
+  schedule.AddGoto(fblock, lblock);
+
+  schedule.AddNode(tblock, if_true);
+  schedule.AddThrow(tblock, throw_node);
+  NodeProperties::MergeControlToEnd(graph(), common(), throw_node);
+
+  ASSERT_THAT(end(), IsEnd(IsThrow()));
+  ASSERT_THAT(end()->op()->ControlInputCount(), 1);
+
+  // Run the state effect linearizer, maintaining the schedule.
+  LinearizeEffectControl(
+      jsgraph(), &schedule, zone(), source_positions(), node_origins(),
+      MaskArrayIndexEnable::kDoNotMaskArrayIndex, MaintainSchedule::kMaintain);
+
+  // Initial block with 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.start()->SuccessorCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorCount(), 1);
+  ASSERT_THAT(schedule.end()->PredecessorAt(0), start);
+}
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8

test/unittests/compiler/node-test-utils.cc

@@ -1489,6 +1489,18 @@ Matcher<Node*> IsDead() {
   return MakeMatcher(new TestNodeMatcher(IrOpcode::kDead));
 }
 
+Matcher<Node*> IsUnreachable() {
+  return MakeMatcher(new TestNodeMatcher(IrOpcode::kUnreachable));
+}
+
+Matcher<Node*> IsThrow() {
+  return MakeMatcher(new TestNodeMatcher(IrOpcode::kThrow));
+}
+
+Matcher<Node*> IsStart() {
+  return MakeMatcher(new TestNodeMatcher(IrOpcode::kStart));
+}
+
 Matcher<Node*> IsEnd(const Matcher<Node*>& control0_matcher) {
   return MakeMatcher(new IsControl1Matcher(IrOpcode::kEnd, control0_matcher));
 }

test/unittests/compiler/node-test-utils.h

@@ -37,6 +37,9 @@ class Node;
 using ::testing::Matcher;
 
 Matcher<Node*> IsDead();
+Matcher<Node*> IsUnreachable();
+Matcher<Node*> IsThrow();
+Matcher<Node*> IsStart();
 Matcher<Node*> IsEnd(const Matcher<Node*>& control0_matcher);
 Matcher<Node*> IsEnd(const Matcher<Node*>& control0_matcher,
                      const Matcher<Node*>& control1_matcher);