[maglev] Fix dead bytecode visit (again)

A few more fixes for visiting bytecodes that are known to be dead:

  * Change JumpLoop dead frame merging to remove the loop predecessor,
    by moving the loop predecessor to be the last in the list and
    allowing it to be dropped the same way as other predecessors.
  * Remove the bytecode walk in EmitUnconditionalDeopt, opting instead
    to check for null current_block in the real bytecode visitor. This
    allows us to handle the case where the start of a basic block is
    dead, but there's no fallthrough into it, so it wouldn't be visited
    by the loop in EmitUnconditionalDeopt.

Bug: v8:7700
Change-Id: I7cf1a54c49a2affc0363c1a0919bb3d427f83f5c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3700070
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81109}

src/maglev/maglev-graph-builder.cc

@@ -1360,7 +1360,7 @@ void MaglevGraphBuilder::VisitJumpLoop() {
 
   merge_states_[target]->MergeLoop(*compilation_unit_,
                                    current_interpreter_frame_, block, target);
-  block->set_predecessor_id(0);
+  block->set_predecessor_id(merge_states_[target]->predecessor_count() - 1);
 }
 void MaglevGraphBuilder::VisitJump() {
   const uint32_t relative_jump_bytecode_offset =
@@ -1418,6 +1418,21 @@ void MaglevGraphBuilder::MergeDeadIntoFrameState(int target) {
   if (merge_states_[target]) {
     // If there already is a frame state, merge.
     merge_states_[target]->MergeDead(*compilation_unit_, target);
+    // If this merge is the last one which kills a loop merge, remove that merge
+    // state.
+    if (merge_states_[target]->is_unreachable_loop()) {
+      merge_states_[target] = nullptr;
+    }
+  }
+}
+
+void MaglevGraphBuilder::MergeDeadLoopIntoFrameState(int target) {
+  // If there is no merge state yet, don't create one, but just reduce the
+  // number of possible predecessors to zero.
+  predecessors_[target]--;
+  if (merge_states_[target]) {
+    // If there already is a frame state, merge.
+    merge_states_[target]->MergeDeadLoop(*compilation_unit_, target);
   }
 }
 

src/maglev/maglev-graph-builder.h

@@ -86,14 +86,15 @@ class MaglevGraphBuilder {
 
     // Set up edge-split.
     int predecessor_index = merge_state.predecessor_count() - 1;
+    if (merge_state.is_unmerged_loop()) {
+      // For loops, the JumpLoop block hasn't been generated yet, and so isn't
+      // in the list of jump targets. IT's the last predecessor, so drop the
+      // index by one.
+      predecessor_index--;
+    }
     BasicBlockRef* old_jump_targets = jump_targets_[offset].Reset();
     while (old_jump_targets != nullptr) {
       BasicBlock* predecessor = merge_state.predecessor_at(predecessor_index);
-      if (predecessor == MergePointInterpreterFrameState::kDeadPredecessor) {
-        // We might have dead predecessors.
-        predecessor_index--;
-        continue;
-      }
       ControlNode* control = predecessor->control_node();
       if (control->Is<ConditionalControlNode>()) {
         // CreateEmptyBlock automatically registers itself with the offset.
@@ -109,17 +110,7 @@ class MaglevGraphBuilder {
       }
       predecessor->set_predecessor_id(predecessor_index--);
     }
-#ifdef DEBUG
-    if (bytecode_analysis().IsLoopHeader(offset)) {
-      // For loops, the JumpLoop block hasn't been generated yet, and so isn't
-      // in the list of jump targets. It's defined to be at index 0, so once
-      // we've processed all the jump targets, the 0 index should be the one
-      // remaining.
-      DCHECK_EQ(predecessor_index, 0);
-    } else {
-      DCHECK_EQ(predecessor_index, -1);
-    }
-#endif
+    DCHECK_EQ(predecessor_index, -1);
     if (has_graph_labeller()) {
       for (Phi* phi : *merge_states_[offset]->phis()) {
         graph_labeller()->RegisterNode(phi);
@@ -140,78 +131,72 @@ class MaglevGraphBuilder {
     BasicBlock* block = CreateBlock<Deopt>({});
     ResolveJumpsToBlockAtOffset(block, block_offset_);
 
-    // Consider any bytecodes from here onwards dead, up to the next merge point
-    // with non-dead predecessors. Any control flow encountered is also
-    // considered dead and should mark its successors as dead.
-    while (true) {
-      // If the current bytecode is a jump to elsewhere, then this jump is
-      // also dead and we should make sure to merge it as a dead predecessor.
-      interpreter::Bytecode bytecode = iterator_.current_bytecode();
-      if (interpreter::Bytecodes::IsForwardJump(bytecode)) {
-        // Jumps merge into their target, and conditional jumps also merge into
-        // the fallthrough.
-        MergeDeadIntoFrameState(iterator_.GetJumpTargetOffset());
-        if (interpreter::Bytecodes::IsConditionalJump(bytecode)) {
-          MergeDeadIntoFrameState(iterator_.next_offset());
-        }
-      } else if (bytecode == interpreter::Bytecode::kJumpLoop) {
-        // JumpLoop merges into its loop header, which has to be treated
-        // specially by the merge..
-        int target = iterator_.GetJumpTargetOffset();
-        merge_states_[target]->MergeDeadLoop();
-      } else if (interpreter::Bytecodes::IsSwitch(bytecode)) {
-        // Switches merge into their targets, and into the fallthrough.
-        for (auto offset : iterator_.GetJumpTableTargetOffsets()) {
-          MergeDeadIntoFrameState(offset.target_offset);
-        }
-        MergeDeadIntoFrameState(iterator_.next_offset());
-      } else if (!interpreter::Bytecodes::Returns(bytecode) &&
-                 !interpreter::Bytecodes::UnconditionallyThrows(bytecode)) {
-        // Any other bytecode that doesn't return or throw will merge into the
-        // fallthrough.
-        MergeDeadIntoFrameState(iterator_.next_offset());
-      }
+    MarkBytecodeDead();
+  }
 
-      // If the next offset is a merge point, that means another live bytecode
-      // created its merge state and it is reachable. We should stop iterating.
-      if (IsOffsetAMergePoint(iterator_.next_offset())) {
-        // The exception is loops that are unreachable aside from their
-        // back-edge. This back-edge will itself not be reachable, thanks to
-        // irreducibility, so in this case the loop header will still be dead.
-        if (!merge_states_[iterator_.next_offset()]->is_unreachable_loop()) {
-          break;
-        }
-      }
+  void MarkBytecodeDead() {
+    DCHECK_NULL(current_block_);
 
-      // Otherwise, move on to the next bytecode. Save the offset in case we
-      // want to rewind the Advance, which we need to do if we fall off the end
-      // of the iterator.
-      // TODO(leszeks): Not having to save the offset would be more elegant, but
-      // then we need to play nicer with the BuildBody loop.
-      int saved_offset = iterator_.current_offset();
-      iterator_.Advance();
-      if (iterator_.done()) {
-        iterator_.SetOffset(saved_offset);
-        break;
+    // If the current bytecode is a jump to elsewhere, then this jump is
+    // also dead and we should make sure to merge it as a dead predecessor.
+    interpreter::Bytecode bytecode = iterator_.current_bytecode();
+    if (interpreter::Bytecodes::IsForwardJump(bytecode)) {
+      // Jumps merge into their target, and conditional jumps also merge into
+      // the fallthrough.
+      MergeDeadIntoFrameState(iterator_.GetJumpTargetOffset());
+      if (interpreter::Bytecodes::IsConditionalJump(bytecode)) {
+        MergeDeadIntoFrameState(iterator_.next_offset());
       }
+    } else if (bytecode == interpreter::Bytecode::kJumpLoop) {
+      // JumpLoop merges into its loop header, which has to be treated
+      // specially by the merge.
+      MergeDeadLoopIntoFrameState(iterator_.GetJumpTargetOffset());
+    } else if (interpreter::Bytecodes::IsSwitch(bytecode)) {
+      // Switches merge into their targets, and into the fallthrough.
+      for (auto offset : iterator_.GetJumpTableTargetOffsets()) {
+        MergeDeadIntoFrameState(offset.target_offset);
+      }
+      MergeDeadIntoFrameState(iterator_.next_offset());
+    } else if (!interpreter::Bytecodes::Returns(bytecode) &&
+               !interpreter::Bytecodes::UnconditionallyThrows(bytecode)) {
+      // Any other bytecode that doesn't return or throw will merge into the
+      // fallthrough.
+      MergeDeadIntoFrameState(iterator_.next_offset());
     }
+
+    // TODO(leszeks): We could now continue iterating the bytecode
   }
 
   void VisitSingleBytecode() {
     int offset = iterator_.current_offset();
-    if (V8_UNLIKELY(merge_states_[offset] != nullptr)) {
+    MergePointInterpreterFrameState* merge_state = merge_states_[offset];
+    if (V8_UNLIKELY(merge_state != nullptr)) {
       if (current_block_ != nullptr) {
         // TODO(leszeks): Re-evaluate this DCHECK, we might hit it if the only
         // bytecodes in this basic block were only register juggling.
         // DCHECK(!current_block_->nodes().is_empty());
         FinishBlock<Jump>(offset, {}, &jump_targets_[offset]);
 
-        merge_states_[offset]->Merge(*compilation_unit_,
-                                     current_interpreter_frame_,
-                                     graph()->last_block(), offset);
+        merge_state->Merge(*compilation_unit_, current_interpreter_frame_,
+                           graph()->last_block(), offset);
       }
       ProcessMergePoint(offset);
       StartNewBlock(offset);
+    } else if (V8_UNLIKELY(current_block_ == nullptr)) {
+      // If we don't have a current block, the bytecode must be dead (because of
+      // some earlier deopt). Mark this bytecode dead too and return.
+      // TODO(leszeks): Merge these two conditions by marking dead states with
+      // a sentinel value.
+#ifdef DEBUG
+      if (predecessors_[offset] == 1) {
+        DCHECK(bytecode_analysis().IsLoopHeader(offset));
+        DCHECK_NULL(merge_state);
+      } else {
+        DCHECK_EQ(predecessors_[offset], 0);
+      }
+#endif
+      MarkBytecodeDead();
+      return;
     }
     DCHECK_NOT_NULL(current_block_);
 #ifdef DEBUG
@@ -623,6 +608,7 @@ class MaglevGraphBuilder {
 
   void MergeIntoFrameState(BasicBlock* block, int target);
   void MergeDeadIntoFrameState(int target);
+  void MergeDeadLoopIntoFrameState(int target);
   void MergeIntoInlinedReturnFrameState(BasicBlock* block);
   void BuildBranchIfTrue(ValueNode* node, int true_target, int false_target);
   void BuildBranchIfToBooleanTrue(ValueNode* node, int true_target,

src/maglev/maglev-graph-printer.cc

@@ -398,12 +398,7 @@ void MaglevPrintingVisitor::Process(Phi* phi, const ProcessingState& state) {
   // moves).
   for (int i = 0; i < phi->input_count(); ++i) {
     if (i > 0) os_ << ", ";
-    if (state.block()->predecessor_at(i) ==
-        MergePointInterpreterFrameState::kDeadPredecessor) {
-      os_ << "<dead>";
-    } else {
-      os_ << PrintNodeLabel(graph_labeller, phi->input(i).node());
-    }
+    os_ << PrintNodeLabel(graph_labeller, phi->input(i).node());
   }
   os_ << ") → " << phi->result().operand() << "\n";
 

src/maglev/maglev-interpreter-frame-state.h

@@ -212,8 +212,6 @@ class MergePointRegisterState {
 
 class MergePointInterpreterFrameState {
  public:
-  static constexpr BasicBlock* kDeadPredecessor = nullptr;
-
   void CheckIsLoopPhiIfNeeded(const MaglevCompilationUnit& compilation_unit,
                               int merge_offset, interpreter::Register reg,
                               ValueNode* value) {
@@ -248,7 +246,7 @@ class MergePointInterpreterFrameState {
       int predecessor_count, const compiler::BytecodeLivenessState* liveness,
       const compiler::LoopInfo* loop_info)
       : predecessor_count_(predecessor_count),
-        predecessors_so_far_(1),
+        predecessors_so_far_(0),
         predecessors_(info.zone()->NewArray<BasicBlock*>(predecessor_count)),
         frame_state_(info, liveness) {
     auto& assignments = loop_info->assignments();
@@ -268,7 +266,7 @@ class MergePointInterpreterFrameState {
         });
     DCHECK(!frame_state_.liveness()->AccumulatorIsLive());
 
-    predecessors_[0] = uninitialized_predecessor();
+    predecessors_[predecessor_count - 1] = unmerged_loop_marker();
   }
 
   // Merges an unmerged framestate with a possibly merged framestate into |this|
@@ -296,9 +294,10 @@ class MergePointInterpreterFrameState {
   void MergeLoop(MaglevCompilationUnit& compilation_unit,
                  const InterpreterFrameState& loop_end_state,
                  BasicBlock* loop_end_block, int merge_offset) {
-    DCHECK_EQ(predecessors_so_far_, predecessor_count_);
-    DCHECK_EQ(predecessors_[0], uninitialized_predecessor());
-    predecessors_[0] = loop_end_block;
+    // This should be the last predecessor we try to merge.
+    DCHECK_EQ(predecessors_so_far_, predecessor_count_ - 1);
+    DCHECK(is_unmerged_loop());
+    predecessors_[predecessor_count_ - 1] = loop_end_block;
 
     frame_state_.ForEachValue(
         compilation_unit, [&](ValueNode* value, interpreter::Register reg) {
@@ -307,6 +306,8 @@ class MergePointInterpreterFrameState {
           MergeLoopValue(compilation_unit, reg, value, loop_end_state.get(reg),
                          merge_offset);
         });
+    predecessors_so_far_++;
+    DCHECK_EQ(predecessors_so_far_, predecessor_count_);
   }
 
   // Merges a dead framestate (e.g. one which has been early terminated with a
@@ -315,6 +316,10 @@ class MergePointInterpreterFrameState {
                  int merge_offset) {
     DCHECK_GT(predecessor_count_, 1);
     DCHECK_LT(predecessors_so_far_, predecessor_count_);
+    // For loops, we need to pull the unmerged loop marker back by one.
+    if (is_unmerged_loop()) {
+      predecessors_[predecessor_count_ - 2] = unmerged_loop_marker();
+    }
     predecessor_count_--;
     DCHECK_LE(predecessors_so_far_, predecessor_count_);
 
@@ -327,10 +332,12 @@ class MergePointInterpreterFrameState {
 
   // Merges a dead loop framestate (e.g. one where the block containing the
   // JumpLoop has been early terminated with a deopt).
-  void MergeDeadLoop() {
-    DCHECK_EQ(predecessors_so_far_, predecessor_count_);
-    DCHECK_EQ(predecessors_[0], uninitialized_predecessor());
-    predecessors_[0] = kDeadPredecessor;
+  void MergeDeadLoop(const MaglevCompilationUnit& compilation_unit,
+                     int merge_offset) {
+    // This should be the last predecessor we try to merge.
+    DCHECK_EQ(predecessors_so_far_, predecessor_count_ - 1);
+    DCHECK(is_unmerged_loop());
+    MergeDead(compilation_unit, merge_offset);
   }
 
   const CompactInterpreterFrameState& frame_state() const {
@@ -355,12 +362,16 @@ class MergePointInterpreterFrameState {
     return predecessors_[i];
   }
 
+  bool is_unmerged_loop() const {
+    DCHECK_GT(predecessor_count_, 0);
+    return predecessors_[predecessor_count_ - 1] == unmerged_loop_marker();
+  }
+
   bool is_unreachable_loop() const {
-    DCHECK_EQ(predecessors_so_far_, predecessor_count_);
     // If there is only one predecessor, and it's not set, then this is a loop
     // merge with no forward control flow entering it.
     return predecessor_count_ == 1 &&
-           predecessors_[0] == uninitialized_predecessor();
+           predecessors_[0] == unmerged_loop_marker();
   }
 
  private:
@@ -368,10 +379,8 @@ class MergePointInterpreterFrameState {
       const MaglevCompilationUnit& info,
       const MergePointInterpreterFrameState& state);
 
-  // Create an uninitialized value sentinel for loop merges. This is distinct
-  // from kDeadPredecessor, which is for loops that were reached, but were dead.
-  // TODO(leszeks): Do this in a way that isn't UB.
-  static BasicBlock* uninitialized_predecessor() {
+  // Create an uninitialized value sentinel for loop merges.
+  static BasicBlock* unmerged_loop_marker() {
     return reinterpret_cast<BasicBlock*>(0x100b);
   }
 
@@ -449,8 +458,8 @@ class MergePointInterpreterFrameState {
     // If the merged node is null, this is a pre-created loop header merge
     // frame will null values for anything that isn't a loop Phi.
     if (merged == nullptr) {
-      DCHECK_EQ(predecessors_[0], uninitialized_predecessor());
-      DCHECK_EQ(predecessors_so_far_, 1);
+      DCHECK(is_unmerged_loop());
+      DCHECK_EQ(predecessors_so_far_, 0);
       return unmerged;
     }
 
@@ -498,8 +507,8 @@ class MergePointInterpreterFrameState {
     // If the merged node is null, this is a pre-created loop header merge
     // frame with null values for anything that isn't a loop Phi.
     if (merged == nullptr) {
-      DCHECK_EQ(predecessors_[0], uninitialized_predecessor());
-      DCHECK_EQ(predecessors_so_far_, 1);
+      DCHECK(is_unmerged_loop());
+      DCHECK_EQ(predecessors_so_far_, 0);
       return;
     }
 
@@ -525,22 +534,15 @@ class MergePointInterpreterFrameState {
       return;
     }
     DCHECK_EQ(result->owner(), owner);
-    // The loop jump is defined to unconditionally be index 0.
-#ifdef DEBUG
-    DCHECK_NULL(result->input(0).node());
-#endif
     unmerged = EnsureTagged(compilation_unit, unmerged);
-    result->set_input(0, unmerged);
+    result->set_input(predecessor_count_ - 1, unmerged);
   }
 
   ValueNode* NewLoopPhi(Zone* zone, interpreter::Register reg,
                         int merge_offset) {
-    DCHECK_EQ(predecessors_so_far_, 1);
+    DCHECK_EQ(predecessors_so_far_, 0);
     // Create a new loop phi, which for now is empty.
     Phi* result = Node::New<Phi>(zone, predecessor_count_, reg, merge_offset);
-#ifdef DEBUG
-    result->set_input(0, nullptr);
-#endif
     phis_.Add(result);
     return result;
   }