[maglev] Prevent register merges clobbering phis

Register merges participate in the same parallel gap move as phi inputs,
but their allocation is not aware of the phis' existence (since the
register merge allocation sees the register state _before_ phi input
allocation, which is because that's what parallel move requires). This
means that they might move into a register that is used by a Phi, and
possibly will clobber its value.

Avoid this by recording what registers phis move values into during code
gen, and skipping register moves into those registers. Also DCHECK that
the recorded gap moves can't clobber a target register from a previous
gap move. Additionally, add printing for register merges (both in
regalloc tracing and graph printing).

Bug: v8:7700
Change-Id: I8bd4803a30a894c5654e33fc5657ef3fe6cf7a0b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3791062Reviewed-by: Victor Gomes <victorgomes@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Victor Gomes <victorgomes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82036}

src/maglev/maglev-code-generator.cc

@@ -268,11 +268,22 @@ class MaglevCodeGeneratingNodeProcessor {
     node->LoadToRegister(code_gen_state_, target);
   }
 
+  template <typename RegisterT>
+  bool HasGapMoveToRegister(RegisterT target_reg,
+                            RegisterMovesT<RegisterT>& register_moves) {
+    for (RegListBase<RegisterT> targets : register_moves) {
+      if (targets.has(target_reg)) return true;
+    }
+    return false;
+  }
+
   template <typename RegisterT>
   void RecordGapMove(ValueNode* node, compiler::InstructionOperand source,
                      RegisterT target_reg,
                      RegisterMovesT<RegisterT>& register_moves,
                      RegisterReloadsT<RegisterT>& register_reloads) {
+    // There shouldn't have been another move to this register already.
+    DCHECK(!HasGapMoveToRegister(target_reg, register_moves));
     if (source.IsAnyRegister()) {
       // For reg->reg moves, don't emit the move yet, but instead record the
       // move in the set of parallel register moves, to be resolved later.
@@ -336,23 +347,9 @@ class MaglevCodeGeneratingNodeProcessor {
     RegisterReloads register_reloads = {};
     DoubleRegisterReloads double_register_reloads = {};
 
-    __ RecordComment("--   Gap moves:");
+    RegList registers_set_by_phis;
 
-    target->state()->register_state().ForEachGeneralRegister(
-        [&](Register reg, RegisterState& state) {
-          ValueNode* node;
-          RegisterMerge* merge;
-          if (LoadMergeState(state, &node, &merge)) {
-            compiler::InstructionOperand source =
-                merge->operand(predecessor_id);
-            if (FLAG_code_comments) {
-              std::stringstream ss;
-              ss << "--   * " << source << " → " << reg;
-              __ RecordComment(ss.str());
-            }
-            RecordGapMove(node, source, reg, register_moves, register_reloads);
-          }
-        });
+    __ RecordComment("--   Gap moves:");
 
     if (target->has_phi()) {
       Phi::List* phis = target->phis();
@@ -383,9 +380,31 @@ class MaglevCodeGeneratingNodeProcessor {
           __ RecordComment(ss.str());
         }
         RecordGapMove(node, source, target, register_moves, register_reloads);
+        if (target.IsAnyRegister()) {
+          registers_set_by_phis.set(target.GetRegister());
+        }
       }
     }
 
+    target->state()->register_state().ForEachGeneralRegister(
+        [&](Register reg, RegisterState& state) {
+          // Don't clobber registers set by a Phi.
+          if (registers_set_by_phis.has(reg)) return;
+
+          ValueNode* node;
+          RegisterMerge* merge;
+          if (LoadMergeState(state, &node, &merge)) {
+            compiler::InstructionOperand source =
+                merge->operand(predecessor_id);
+            if (FLAG_code_comments) {
+              std::stringstream ss;
+              ss << "--   * " << source << " → " << reg;
+              __ RecordComment(ss.str());
+            }
+            RecordGapMove(node, source, reg, register_moves, register_reloads);
+          }
+        });
+
 #define EMIT_MOVE_FOR_REG(Name) EmitParallelMoveChain(Name, register_moves);
     ALLOCATABLE_GENERAL_REGISTERS(EMIT_MOVE_FOR_REG)
 #undef EMIT_MOVE_FOR_REG

src/maglev/maglev-graph-printer.cc

@@ -518,6 +518,27 @@ void MaglevPrintingVisitor::Process(ControlNode* control_node,
         os_ << " → " << graph_labeller->NodeId(phi) << ": Phi "
             << phi->result().operand() << "\n";
       }
+      if (target->state()->register_state().is_initialized()) {
+        PrintVerticalArrows(os_, targets_);
+        PrintPadding(os_, graph_labeller, max_node_id_, -1);
+        os_ << (has_fallthrough ? "│" : " ");
+        os_ << "  with register merges:\n";
+        auto print_register_merges = [&](auto reg, RegisterState& state) {
+          ValueNode* node;
+          RegisterMerge* merge;
+          if (LoadMergeState(state, &node, &merge)) {
+            compiler::InstructionOperand source = merge->operand(pid);
+            PrintVerticalArrows(os_, targets_);
+            PrintPadding(os_, graph_labeller, max_node_id_, -1);
+            os_ << (has_fallthrough ? "│" : " ");
+            os_ << "    - " << source << " → " << reg << "\n";
+          }
+        };
+        target->state()->register_state().ForEachGeneralRegister(
+            print_register_merges);
+        target->state()->register_state().ForEachDoubleRegister(
+            print_register_merges);
+      }
     }
   }
 

src/maglev/maglev-regalloc.cc

@@ -1487,6 +1487,10 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
     return InitializeBranchTargetRegisterValues(control, target);
   }
 
+  if (FLAG_trace_maglev_regalloc) {
+    printing_visitor_->os() << "Merging registers...\n";
+  }
+
   int predecessor_count = target->state()->predecessor_count();
   auto merge = [&](auto& registers, auto reg, RegisterState& state) {
     ValueNode* node;
@@ -1507,6 +1511,11 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
     if (!registers.free().has(reg)) {
       incoming = registers.GetValue(reg);
       if (!IsLiveAtTarget(incoming, control, target)) {
+        if (FLAG_trace_maglev_regalloc) {
+          printing_visitor_->os() << "  " << reg << " - incoming node "
+                                  << PrintNodeLabel(graph_labeller(), incoming)
+                                  << " dead at target\n";
+        }
         incoming = nullptr;
       }
     }
@@ -1514,6 +1523,13 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
     if (incoming == node) {
       // We're using the same register as the target already has. If registers
       // are merged, add input information.
+      if (FLAG_trace_maglev_regalloc) {
+        if (node) {
+          printing_visitor_->os()
+              << "  " << reg << " - incoming node same as node: "
+              << PrintNodeLabel(graph_labeller(), node) << "\n";
+        }
+      }
       if (merge) merge->operand(predecessor_id) = register_info;
       return;
     }
@@ -1522,6 +1538,11 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
       // The register is already occupied with a different node. Figure out
       // where that node is allocated on the incoming branch.
       merge->operand(predecessor_id) = node->allocation();
+      if (FLAG_trace_maglev_regalloc) {
+        printing_visitor_->os() << "  " << reg << " - merge: loading "
+                                << PrintNodeLabel(graph_labeller(), node)
+                                << " from " << node->allocation() << " \n";
+      }
 
       // If there's a value in the incoming state, that value is either
       // already spilled or in another place in the merge state.
@@ -1542,6 +1563,11 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
       // containing conversion nodes.
       // DCHECK_IMPLIES(!IsInRegister(target_state, incoming),
       //                incoming->properties().is_conversion());
+      if (FLAG_trace_maglev_regalloc) {
+        printing_visitor_->os()
+            << "  " << reg << " - can't load incoming "
+            << PrintNodeLabel(graph_labeller(), node) << ", bailing out\n";
+      }
       return;
     }
 
@@ -1552,6 +1578,11 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
       // over the liveness of the node they are converting.
       // TODO(v8:7700): Overeager DCHECK.
       // DCHECK(node->properties().is_conversion());
+      if (FLAG_trace_maglev_regalloc) {
+        printing_visitor_->os() << "  " << reg << " - can't load "
+                                << PrintNodeLabel(graph_labeller(), node)
+                                << ", dropping the merge\n";
+      }
       state = {nullptr, initialized_node};
       return;
     }
@@ -1579,8 +1610,18 @@ void StraightForwardRegisterAllocator::MergeRegisterValues(ControlNode* control,
     // state.
     if (node == nullptr) {
       merge->operand(predecessor_id) = register_info;
+      if (FLAG_trace_maglev_regalloc) {
+        printing_visitor_->os() << "  " << reg << " - new merge: loading new "
+                                << PrintNodeLabel(graph_labeller(), incoming)
+                                << " from " << register_info << " \n";
+      }
     } else {
       merge->operand(predecessor_id) = node->allocation();
+      if (FLAG_trace_maglev_regalloc) {
+        printing_visitor_->os() << "  " << reg << " - new merge: loading "
+                                << PrintNodeLabel(graph_labeller(), node)
+                                << " from " << node->allocation() << " \n";
+      }
     }
     state = {merge, initialized_merge};
   };