Improve usability of GraphAssembler's Unreachable()

Prior to this CL, one had to artificially insert a
basic-block-terminating node after Unreachable. The common pattern was

 Unreachable();
 Goto(&some_label);  // Never reached but generates useless code.

This CL improves usability by automatically merging Unreachable nodes
to the end node, and terminating current effect/control. The updated
pattern is just

 Unreachable();

or in cases where Turboprop must maintain a schedule:

 Unreachable(&some_label);

Bug: v8:8888
Change-Id: I26a0b11b5e67252a6dc3584ae09ed06370f1eacc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2362690
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69531}

src/compiler/effect-control-linearizer.cc

@@ -2599,8 +2599,7 @@ Node* EffectControlLinearizer::LowerCheckedUint32Bounds(Node* node,
     __ Branch(check, &done, &if_abort);
 
     __ Bind(&if_abort);
-    __ Unreachable();
-    __ Goto(&done);
+    __ Unreachable(&done);
 
     __ Bind(&done);
   }
@@ -2646,8 +2645,7 @@ Node* EffectControlLinearizer::LowerCheckedUint64Bounds(Node* node,
     __ Branch(check, &done, &if_abort);
 
     __ Bind(&if_abort);
-    __ Unreachable();
-    __ Goto(&done);
+    __ Unreachable(&done);
 
     __ Bind(&done);
   }
@@ -4019,7 +4017,11 @@ Node* EffectControlLinearizer::LowerNumberSameValue(Node* node) {
 Node* EffectControlLinearizer::LowerDeadValue(Node* node) {
   Node* input = NodeProperties::GetValueInput(node, 0);
   if (input->opcode() != IrOpcode::kUnreachable) {
-    Node* unreachable = __ Unreachable();
+    // There is no fundamental reason not to connect to end here, except it
+    // integrates into the way the graph is constructed in a simpler way at
+    // this point.
+    // TODO(jgruber): Connect to end here as well.
+    Node* unreachable = __ UnreachableWithoutConnectToEnd();
     NodeProperties::ReplaceValueInput(node, unreachable, 0);
   }
   return gasm()->AddNode(node);
@@ -5698,9 +5700,7 @@ void EffectControlLinearizer::LowerTransitionAndStoreNumberElement(Node* node) {
     // loop peeling can break this assumption.
     __ GotoIf(__ Word32Equal(kind, __ Int32Constant(HOLEY_DOUBLE_ELEMENTS)),
               &do_store);
-    // TODO(turbofan): It would be good to have an "Unreachable()" node type.
-    __ DebugBreak();
-    __ Goto(&do_store);
+    __ Unreachable(&do_store);
   }
 
   __ Bind(&transition_smi_array);  // deferred code.

src/compiler/graph-assembler.cc

@@ -624,7 +624,20 @@ Node* GraphAssembler::DebugBreak() {
       graph()->NewNode(machine()->DebugBreak(), effect(), control()));
 }
 
-Node* GraphAssembler::Unreachable() {
+Node* GraphAssembler::Unreachable(
+    GraphAssemblerLabel<0u>* block_updater_successor) {
+  Node* result = UnreachableWithoutConnectToEnd();
+  if (block_updater_ == nullptr) {
+    ConnectUnreachableToEnd();
+    InitializeEffectControl(nullptr, nullptr);
+  } else {
+    DCHECK_NOT_NULL(block_updater_successor);
+    Goto(block_updater_successor);
+  }
+  return result;
+}
+
+Node* GraphAssembler::UnreachableWithoutConnectToEnd() {
   return AddNode(
       graph()->NewNode(common()->Unreachable(), effect(), control()));
 }
@@ -860,7 +873,7 @@ void GraphAssembler::ConnectUnreachableToEnd() {
   // 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.
+  // subsequent unreachable code from the schedule.
   if (!block_updater_) {
     Node* throw_node = graph()->NewNode(common()->Throw(), effect(), control());
     NodeProperties::MergeControlToEnd(graph(), common(), throw_node);

src/compiler/graph-assembler.h

@@ -252,10 +252,20 @@ class V8_EXPORT_PRIVATE GraphAssembler {
   CHECKED_ASSEMBLER_MACH_BINOP_LIST(BINOP_DECL)
 #undef BINOP_DECL
 
-  // Debugging
   Node* DebugBreak();
 
-  Node* Unreachable();
+  // Unreachable nodes are similar to Goto in that they reset effect/control to
+  // nullptr and it's thus not possible to append other nodes without first
+  // binding a new label.
+  // The block_updater_successor label is a crutch to work around block updater
+  // weaknesses (see the related comment in ConnectUnreachableToEnd); if the
+  // block updater exists, we cannot connect unreachable to end, instead we
+  // must preserve the Goto pattern.
+  Node* Unreachable(GraphAssemblerLabel<0u>* block_updater_successor = nullptr);
+  // This special variant doesn't connect the Unreachable node to end, and does
+  // not reset current effect/control. Intended only for special use-cases like
+  // lowering DeadValue.
+  Node* UnreachableWithoutConnectToEnd();
 
   Node* IntPtrEqual(Node* left, Node* right);
   Node* TaggedEqual(Node* left, Node* right);
@@ -350,6 +360,13 @@ class V8_EXPORT_PRIVATE GraphAssembler {
   void GotoIfNot(Node* condition, GraphAssemblerLabel<sizeof...(Vars)>* label,
                  Vars...);
 
+  bool HasActiveBlock() const {
+    // This is false if the current block has been terminated (e.g. by a Goto or
+    // Unreachable). In that case, a new label must be bound before we can
+    // continue emitting nodes.
+    return control() != nullptr;
+  }
+
   // Updates current effect and control based on outputs of {node}.
   V8_INLINE void UpdateEffectControlWith(Node* node) {
     if (node->op()->EffectOutputCount() > 0) {
@@ -375,8 +392,8 @@ class V8_EXPORT_PRIVATE GraphAssembler {
 
   void ConnectUnreachableToEnd();
 
-  Control control() { return Control(control_); }
-  Effect effect() { return Effect(effect_); }
+  Control control() const { return Control(control_); }
+  Effect effect() const { return Effect(effect_); }
 
  protected:
   class BasicBlockUpdater;

src/compiler/js-call-reducer.cc

@@ -143,11 +143,11 @@ class JSCallReducerAssembler : public JSGraphAssembler {
 
       gasm_->Bind(&if_true);
       if (then_body_) then_body_();
-      gasm_->Goto(&merge);
+      if (gasm_->HasActiveBlock()) gasm_->Goto(&merge);
 
       gasm_->Bind(&if_false);
       if (else_body_) else_body_();
-      gasm_->Goto(&merge);
+      if (gasm_->HasActiveBlock()) gasm_->Goto(&merge);
 
       gasm_->Bind(&merge);
     }
@@ -209,11 +209,13 @@ class JSCallReducerAssembler : public JSGraphAssembler {
 
       gasm_->Bind(&if_true);
       TNode<T> then_result = then_body_();
-      gasm_->Goto(&merge, then_result);
+      if (gasm_->HasActiveBlock()) gasm_->Goto(&merge, then_result);
 
       gasm_->Bind(&if_false);
       TNode<T> else_result = else_body_();
-      gasm_->Goto(&merge, else_result);
+      if (gasm_->HasActiveBlock()) {
+        gasm_->Goto(&merge, else_result);
+      }
 
       gasm_->Bind(&merge);
       return merge.PhiAt<T>(0);
@@ -1420,7 +1422,6 @@ TNode<Object> IteratingArrayBuiltinReducerAssembler::ReduceArrayPrototypeReduce(
       Bind(&continue_label);
     });
     Unreachable();  // The loop is exited either by deopt or a jump to below.
-    InitializeEffectControl(nullptr, nullptr);
 
     // TODO(jgruber): This manual fiddling with blocks could be avoided by
     // implementing a `break` mechanic for loop builders.

src/compiler/wasm-compiler.cc

@@ -134,15 +134,18 @@ MachineType assert_size(int expected_size, MachineType type) {
             wasm::ObjectAccess::ElementOffsetInTaggedFixedArray(index), value, \
             MachineRepresentation::kTagged, kFullWriteBarrier)
 
-void MergeControlToEnd(MachineGraph* mcgraph, Node* node) {
+void EnsureEnd(MachineGraph* mcgraph) {
   Graph* g = mcgraph->graph();
-  if (g->end()) {
-    NodeProperties::MergeControlToEnd(g, mcgraph->common(), node);
-  } else {
-    g->SetEnd(g->NewNode(mcgraph->common()->End(1), node));
+  if (g->end() == nullptr) {
+    g->SetEnd(g->NewNode(mcgraph->common()->End(0)));
   }
 }
 
+void MergeControlToEnd(MachineGraph* mcgraph, Node* node) {
+  EnsureEnd(mcgraph);
+  NodeProperties::MergeControlToEnd(mcgraph->graph(), mcgraph->common(), node);
+}
+
 bool ContainsSimd(const wasm::FunctionSig* sig) {
   for (auto type : sig->all()) {
     if (type == wasm::kWasmS128) return true;
@@ -5568,13 +5571,13 @@ Node* IsI31(GraphAssembler* gasm, Node* object) {
   }
 }
 
-void AssertFalse(GraphAssembler* gasm, Node* condition) {
+void AssertFalse(MachineGraph* mcgraph, GraphAssembler* gasm, Node* condition) {
 #if DEBUG
   if (FLAG_debug_code) {
     auto ok = gasm->MakeLabel();
     gasm->GotoIfNot(condition, &ok);
+    EnsureEnd(mcgraph);
     gasm->Unreachable();
-    gasm->Goto(&ok);
     gasm->Bind(&ok);
   }
 #endif
@@ -5592,7 +5595,7 @@ Node* WasmGraphBuilder::RefTest(Node* object, Node* rtt,
     gasm_->GotoIf(IsI31(gasm_.get(), object), &done, gasm_->Int32Constant(0));
     need_done_label = true;
   } else {
-    AssertFalse(gasm_.get(), IsI31(gasm_.get(), object));
+    AssertFalse(mcgraph(), gasm_.get(), IsI31(gasm_.get(), object));
   }
   if (null_check == kWithNullCheck) {
     gasm_->GotoIf(gasm_->WordEqual(object, RefNull()), &done,
@@ -5627,7 +5630,7 @@ Node* WasmGraphBuilder::RefCast(Node* object, Node* rtt,
       TrapIfTrue(wasm::kTrapIllegalCast, IsI31(gasm_.get(), object), position);
     }
   } else {
-    AssertFalse(gasm_.get(), IsI31(gasm_.get(), object));
+    AssertFalse(mcgraph(), gasm_.get(), IsI31(gasm_.get(), object));
   }
   if (null_check == kWithNullCheck) {
     TrapIfTrue(wasm::kTrapIllegalCast, gasm_->WordEqual(object, RefNull()),
@@ -5667,7 +5670,7 @@ Node* WasmGraphBuilder::BrOnCast(Node* object, Node* rtt,
       merge_effects.emplace_back(effect());
     }
   } else {
-    AssertFalse(gasm_.get(), is_i31);
+    AssertFalse(mcgraph(), gasm_.get(), is_i31);
   }
 
   if (null_check == kWithNullCheck) {