From d9828e4553f34df6b5ec9040852670cb003e951e Mon Sep 17 00:00:00 2001
From: Ross McIlroy <rmcilroy@chromium.org>
Date: Tue, 12 May 2020 21:27:09 +0100
Subject: [PATCH] [Turboprop] Allow removal of multiple unreachable blocks that
 merge.

The scheduler could schedule unreachable nodes on two basic blocks that
later merge. Update DCHECK in graph-assembler's basic block updater to
only check for the self-containedness of unreachable basic blocks
removed from the schedule after all the blocks have been re-written to
allow for this case.

BUG=chromium:1079446,v8:9684

Change-Id: I91899dbf389e4425542dbd2b1ca95c3f6ad79c05
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2196354
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67812}
---
 src/compiler/graph-assembler.cc         | 38 ++++++++++++++-----------
 test/mjsunit/regress/regress-1079446.js | 17 +++++++++++
 2 files changed, 39 insertions(+), 16 deletions(-)
 create mode 100644 test/mjsunit/regress/regress-1079446.js

diff --git a/src/compiler/graph-assembler.cc b/src/compiler/graph-assembler.cc
index 6057f1ce64..b5fac343df 100644
--- a/src/compiler/graph-assembler.cc
+++ b/src/compiler/graph-assembler.cc
@@ -21,6 +21,7 @@ class GraphAssembler::BasicBlockUpdater {
  public:
   BasicBlockUpdater(Schedule* schedule, Graph* graph,
                     CommonOperatorBuilder* common, Zone* temp_zone);
+  ~BasicBlockUpdater();
 
   Node* AddNode(Node* node);
   Node* AddNode(Node* node, BasicBlock* to);
@@ -85,6 +86,9 @@ class GraphAssembler::BasicBlockUpdater {
   bool original_deferred_;
   size_t original_node_count_;
 
+  // Blocks which have been removed from the schedule due to being unreachable.
+  ZoneSet<BasicBlock*> unreachable_blocks_;
+
   State state_;
 };
 
@@ -103,8 +107,20 @@ GraphAssembler::BasicBlockUpdater::BasicBlockUpdater(
       original_control_input_(nullptr),
       original_deferred_(false),
       original_node_count_(graph->NodeCount()),
+      unreachable_blocks_(temp_zone),
       state_(kUnchanged) {}
 
+GraphAssembler::BasicBlockUpdater::~BasicBlockUpdater() {
+#ifdef DEBUG
+  // Ensure that the set of blocks being removed from the schedule are self
+  // contained, i.e., all predecessors have been removed from these blocks.
+  for (BasicBlock* block : unreachable_blocks_) {
+    CHECK_EQ(block->PredecessorCount(), 0);
+    CHECK_EQ(block->SuccessorCount(), 0);
+  }
+#endif
+}
+
 Node* GraphAssembler::BasicBlockUpdater::AddNode(Node* node) {
   return AddNode(node, current_block_);
 }
@@ -269,13 +285,12 @@ void GraphAssembler::BasicBlockUpdater::AddGoto(BasicBlock* from,
 }
 
 void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
-  ZoneSet<BasicBlock*> blocks(temp_zone());
   ZoneQueue<BasicBlock*> worklist(temp_zone());
 
   for (SuccessorInfo succ : saved_successors_) {
     BasicBlock* block = succ.block;
     block->predecessors().erase(block->predecessors().begin() + succ.index);
-    blocks.insert(block);
+    unreachable_blocks_.insert(block);
     worklist.push(block);
   }
   saved_successors_.clear();
@@ -283,10 +298,10 @@ void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
   // Walk through blocks until we get to the end node, then remove the path from
   // 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
-  // an effect-phi. We would need to extend this if we need the ability to mark
-  // control flow as unreachable later in the pipeline.
+  // that doesn't re-merge with reachable control flow (checked in the
+  // destructor) 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();
@@ -306,22 +321,13 @@ void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
                                              current->control_input());
       } else {
         // Otherwise, add successor to worklist if it's not already been seen.
-        if (blocks.insert(successor).second) {
+        if (unreachable_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, i.e., all predecessors have been removed from these blocks.
-  for (BasicBlock* block : blocks) {
-    CHECK_EQ(block->PredecessorCount(), 0);
-    CHECK_EQ(block->SuccessorCount(), 0);
-  }
-#endif
 }
 
 void GraphAssembler::BasicBlockUpdater::AddThrow(Node* node) {
diff --git a/test/mjsunit/regress/regress-1079446.js b/test/mjsunit/regress/regress-1079446.js
new file mode 100644
index 0000000000..2322843751
--- /dev/null
+++ b/test/mjsunit/regress/regress-1079446.js
@@ -0,0 +1,17 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --turboprop
+
+arr = new Int16Array();
+function foo() {
+  arr.__defineGetter__('a', function() { });
+  arr[0] = "123.12";
+}
+
+%PrepareFunctionForOptimization(foo);
+foo();
+foo();
+%OptimizeFunctionOnNextCall(foo);
+foo();