[TurboProp] Don't try to rewire unreachable blocks to end.

We can't consistently rewire the successor blocks of an unreachable node to
disconnect them from the graph when we are trying to maintain the schedule.
Instead simply leave the code there. As a future optimization we could add a
proper scheduled dead code elimination phase which can deal with this.

As a side-effect, one of the tests sees a int64 DeadValue, so add support for that
in the instruction selector.

BUG=chromium:1083272,chromium:1083763,chromium:1084953,v8:9684

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

src/compiler/backend/instruction-selector-impl.h

@@ -356,6 +356,8 @@ class OperandGenerator {
           case MachineRepresentation::kCompressed:
           case MachineRepresentation::kCompressedPointer:
             return Constant(static_cast<int32_t>(0));
+          case MachineRepresentation::kWord64:
+            return Constant(static_cast<int64_t>(0));
           case MachineRepresentation::kFloat64:
             return Constant(static_cast<double>(0));
           case MachineRepresentation::kFloat32:

src/compiler/effect-control-linearizer.cc

@@ -3756,7 +3756,7 @@ Node* EffectControlLinearizer::LowerDeadValue(Node* node) {
     Node* unreachable = __ Unreachable();
     NodeProperties::ReplaceValueInput(node, unreachable, 0);
   }
-  return node;
+  return gasm()->AddNode(node);
 }
 
 Node* EffectControlLinearizer::LowerStringToNumber(Node* node) {

src/compiler/graph-assembler.cc

@@ -21,7 +21,6 @@ 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);
@@ -33,7 +32,6 @@ class GraphAssembler::BasicBlockUpdater {
   void AddBranch(Node* branch, BasicBlock* tblock, BasicBlock* fblock);
   void AddGoto(BasicBlock* to);
   void AddGoto(BasicBlock* from, BasicBlock* to);
-  void AddThrow(Node* node);
 
   void StartBlock(BasicBlock* block);
   BasicBlock* Finalize(BasicBlock* original);
@@ -86,9 +84,6 @@ 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_;
 };
 
@@ -107,20 +102,8 @@ 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_);
 }
@@ -284,82 +267,6 @@ void GraphAssembler::BasicBlockUpdater::AddGoto(BasicBlock* from,
   current_block_ = nullptr;
 }
 
-void GraphAssembler::BasicBlockUpdater::RemoveSuccessorsFromSchedule() {
-  ZoneQueue<BasicBlock*> worklist(temp_zone());
-
-  for (SuccessorInfo succ : saved_successors_) {
-    BasicBlock* block = succ.block;
-    block->predecessors().erase(block->predecessors().begin() + succ.index);
-    unreachable_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, 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 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();
-
-    for (BasicBlock* successor : current->successors()) {
-      // 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);
-        NodeProperties::RemoveControlFromEnd(graph_, common_,
-                                             current->control_input());
-      } else {
-        // Otherwise, add successor to worklist if it's not already been seen.
-        if (unreachable_blocks_.insert(successor).second) {
-          worklist.push(successor);
-        }
-      }
-    }
-    current->ClearSuccessors();
-  }
-}
-
-void GraphAssembler::BasicBlockUpdater::AddThrow(Node* node) {
-  if (state_ == kUnchanged) {
-    CopyForChange();
-  }
-
-  // Clear original successors and replace the block's original control and
-  // control input to the throw, since this block is now connected directly to
-  // the end.
-  if (original_control_input_ != nullptr) {
-    NodeProperties::ReplaceUses(original_control_input_, node, nullptr, node);
-    original_control_input_->Kill();
-  }
-  original_control_input_ = node;
-  original_control_ = BasicBlock::kThrow;
-
-  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();
-
-    // 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});
-  }
-}
-
 void GraphAssembler::BasicBlockUpdater::UpdateSuccessors(BasicBlock* block) {
   for (SuccessorInfo succ : saved_successors_) {
     (succ.block->predecessors())[succ.index] = block;
@@ -912,11 +819,15 @@ BasicBlock* GraphAssembler::FinalizeCurrentBlock(BasicBlock* block) {
 
 void GraphAssembler::ConnectUnreachableToEnd() {
   DCHECK_EQ(effect()->opcode(), IrOpcode::kUnreachable);
-  Node* throw_node = graph()->NewNode(common()->Throw(), effect(), control());
-  NodeProperties::MergeControlToEnd(graph(), common(), throw_node);
-  effect_ = control_ = mcgraph()->Dead();
-  if (block_updater_) {
-    block_updater_->AddThrow(throw_node);
+  // When maintaining the schedule we can't easily rewire the successor blocks
+  // to disconnect them from the graph, so we just leave the unreachable nodes
+  // in the schedule.
+  // TODO(9684): Add a scheduled dead-code elimination phase to remove all the
+  // subsiquent unreacahble code from the schedule.
+  if (!block_updater_) {
+    Node* throw_node = graph()->NewNode(common()->Throw(), effect(), control());
+    NodeProperties::MergeControlToEnd(graph(), common(), throw_node);
+    effect_ = control_ = mcgraph()->Dead();
   }
 }
 

test/mjsunit/regress/regress-1083272.js

+// 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
+
+function foo(e, t) {
+    for (var n = [e], s = e.length; s > 0; --s) {}
+    for (var s = 0; s < n.length; s++) { t() }
+}
+
+var e = 'abc';
+function t() {};
+
+%PrepareFunctionForOptimization(foo);
+foo(e, t);
+foo(e, t);
+%OptimizeFunctionOnNextCall(foo);
+foo(e, t);

test/mjsunit/regress/regress-1083763.js

+// 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
+
+function bar() {}
+
+function foo() {
+  try {
+    bar( "abc".charAt(4));
+  } catch (e) {}
+}
+
+%PrepareFunctionForOptimization(foo);
+foo();
+foo();
+%OptimizeFunctionOnNextCall(foo);
+foo();

test/mjsunit/regress/regress-1084953.js

+// 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
+
+function foo() {
+  try {
+    +Symbol();
+  } catch {
+  }
+}
+%PrepareFunctionForOptimization(foo);
+foo();
+%OptimizeFunctionOnNextCall(foo);
+foo();

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

@@ -339,13 +339,7 @@ TEST_F(EffectControlLinearizerTest, UnreachableThenBranch) {
       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) {
@@ -397,13 +391,7 @@ TEST_F(EffectControlLinearizerTest, UnreachableThenDiamond) {
       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) {
@@ -460,13 +448,7 @@ TEST_F(EffectControlLinearizerTest, UnreachableThenLoop) {
       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, UnreachableInChangedBlockThenBranch) {
@@ -519,12 +501,7 @@ TEST_F(EffectControlLinearizerTest, UnreachableInChangedBlockThenBranch) {
       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