[maglev] Improve graph printing and visiting

Centralise constant visiting into the graph visitor, and adapt graph
printing to print both the labeller node id, and the node numbering node
id.

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

src/maglev/maglev-compiler.cc

@@ -47,28 +47,7 @@ namespace maglev {
 
 class NumberingProcessor {
  public:
-  void PreProcessGraph(MaglevCompilationInfo*, Graph* graph) {
-    node_id_ = 1;
-    for (Constant* constant : graph->constants()) {
-      constant->set_id(node_id_++);
-    }
-    for (const auto& [index, constant] : graph->root()) {
-      constant->set_id(node_id_++);
-      USE(index);
-    }
-    for (const auto& [index, constant] : graph->smi()) {
-      constant->set_id(node_id_++);
-      USE(index);
-    }
-    for (const auto& [index, constant] : graph->int32()) {
-      constant->set_id(node_id_++);
-      USE(index);
-    }
-    for (const auto& [index, constant] : graph->float64()) {
-      constant->set_id(node_id_++);
-      USE(index);
-    }
-  }
+  void PreProcessGraph(MaglevCompilationInfo*, Graph* graph) { node_id_ = 1; }
   void PostProcessGraph(MaglevCompilationInfo*, Graph* graph) {}
   void PreProcessBasicBlock(MaglevCompilationInfo*, BasicBlock* block) {}
 

src/maglev/maglev-graph-labeller.h

@@ -33,8 +33,6 @@ class MaglevGraphLabeller {
 
   int max_node_id() const { return next_node_id_ - 1; }
 
-  int max_node_id_width() const { return std::ceil(std::log10(max_node_id())); }
-
   void PrintNodeLabel(std::ostream& os, const Node* node) {
     auto node_id_it = node_ids_.find(node);
 
@@ -43,6 +41,9 @@ class MaglevGraphLabeller {
       return;
     }
 
+    if (node->has_id()) {
+      os << "v" << node->id() << "/";
+    }
     os << "n" << node_id_it->second;
   }
 

src/maglev/maglev-graph-printer.h

@@ -10,6 +10,8 @@
 #include <set>
 #include <vector>
 
+#include "src/maglev/maglev-ir.h"
+
 namespace v8 {
 namespace internal {
 namespace maglev {
@@ -42,6 +44,7 @@ class MaglevPrintingVisitor {
   std::unique_ptr<std::ostream> os_for_additional_info_;
   std::set<BasicBlock*> loop_headers_;
   std::vector<BasicBlock*> targets_;
+  NodeIdT max_node_id_ = kInvalidNodeId;
 };
 
 void PrintGraph(std::ostream& os, MaglevCompilationInfo* compilation_info,

src/maglev/maglev-graph-processor.h

@@ -82,6 +82,26 @@ class GraphProcessor {
 
     node_processor_.PreProcessGraph(compilation_info_, graph);
 
+    for (Constant* constant : graph->constants()) {
+      node_processor_.Process(constant, GetCurrentState());
+    }
+    for (const auto& [index, constant] : graph->root()) {
+      node_processor_.Process(constant, GetCurrentState());
+      USE(index);
+    }
+    for (const auto& [index, constant] : graph->smi()) {
+      node_processor_.Process(constant, GetCurrentState());
+      USE(index);
+    }
+    for (const auto& [index, constant] : graph->int32()) {
+      node_processor_.Process(constant, GetCurrentState());
+      USE(index);
+    }
+    for (const auto& [index, constant] : graph->float64()) {
+      node_processor_.Process(constant, GetCurrentState());
+      USE(index);
+    }
+
     for (block_it_ = graph->begin(); block_it_ != graph->end(); ++block_it_) {
       BasicBlock* block = *block_it_;
 

src/maglev/maglev-ir.h

@@ -582,6 +582,7 @@ class NodeBase : public ZoneObject {
     return std::make_reverse_iterator(input_address(input_count() - 1));
   }
 
+  constexpr bool has_id() const { return id_ != kInvalidNodeId; }
   constexpr NodeIdT id() const {
     DCHECK_NE(id_, kInvalidNodeId);
     return id_;

src/maglev/maglev-regalloc.cc

@@ -222,7 +222,7 @@ void StraightForwardRegisterAllocator::PrintLiveRegs() const {
     } else {
       printing_visitor_->os() << ", ";
     }
-    printing_visitor_->os() << reg << "=";
+    printing_visitor_->os() << reg << "=v" << node->id();
   };
   general_registers_.ForEachUsedRegister(print);
   double_registers_.ForEachUsedRegister(print);
@@ -939,44 +939,54 @@ void StraightForwardRegisterAllocator::VerifyInputs(NodeBase* node) {
 
 void StraightForwardRegisterAllocator::VerifyRegisterState() {
 #ifdef DEBUG
+  auto NodeNameForFatal = [&](ValueNode* node) {
+    std::stringstream ss;
+    if (compilation_info_->has_graph_labeller()) {
+      ss << PrintNodeLabel(compilation_info_->graph_labeller(), node);
+    } else {
+      ss << "<" << node << ">";
+    }
+    return ss.str();
+  };
+
   for (Register reg : general_registers_.used()) {
     ValueNode* node = general_registers_.GetValue(reg);
     // We shouldn't have any blocked registers by now.
     if (!node->is_in_register(reg)) {
-      FATAL("Node n%d doesn't think it is in register %s",
-            graph_labeller()->NodeId(node), RegisterName(reg));
+      FATAL("Node %s doesn't think it is in register %s",
+            NodeNameForFatal(node).c_str(), RegisterName(reg));
     }
   }
   for (DoubleRegister reg : double_registers_.used()) {
     ValueNode* node = double_registers_.GetValue(reg);
     // We shouldn't have any blocked registers by now.
     if (!node->is_in_register(reg)) {
-      FATAL("Node n%d doesn't think it is in register %s",
-            graph_labeller()->NodeId(node), RegisterName(reg));
+      FATAL("Node %s doesn't think it is in register %s",
+            NodeNameForFatal(node).c_str(), RegisterName(reg));
     }
   }
 
-  auto ValidateValueNode = [this](ValueNode* node) {
+  auto ValidateValueNode = [this, NodeNameForFatal](ValueNode* node) {
     if (node->use_double_register()) {
       for (DoubleRegister reg : node->result_registers<DoubleRegister>()) {
         if (double_registers_.free().has(reg)) {
-          FATAL("Node n%d thinks it's in register %s but it's free",
-                graph_labeller()->NodeId(node), RegisterName(reg));
+          FATAL("Node %s thinks it's in register %s but it's free",
+                NodeNameForFatal(node).c_str(), RegisterName(reg));
         } else if (double_registers_.GetValue(reg) != node) {
-          FATAL("Node n%d thinks it's in register %s but it contains n%d",
-                graph_labeller()->NodeId(node), RegisterName(reg),
-                graph_labeller()->NodeId(double_registers_.GetValue(reg)));
+          FATAL("Node %s thinks it's in register %s but it contains %s",
+                NodeNameForFatal(node).c_str(), RegisterName(reg),
+                NodeNameForFatal(double_registers_.GetValue(reg)).c_str());
         }
       }
     } else {
       for (Register reg : node->result_registers<Register>()) {
         if (general_registers_.free().has(reg)) {
-          FATAL("Node n%d thinks it's in register %s but it's free",
-                graph_labeller()->NodeId(node), RegisterName(reg));
+          FATAL("Node %s thinks it's in register %s but it's free",
+                NodeNameForFatal(node).c_str(), RegisterName(reg));
         } else if (general_registers_.GetValue(reg) != node) {
-          FATAL("Node n%d thinks it's in register %s but it contains n%d",
-                graph_labeller()->NodeId(node), RegisterName(reg),
-                graph_labeller()->NodeId(general_registers_.GetValue(reg)));
+          FATAL("Node %s thinks it's in register %s but it contains %s",
+                NodeNameForFatal(node).c_str(), RegisterName(reg),
+                NodeNameForFatal(general_registers_.GetValue(reg)).c_str());
         }
       }
     }
@@ -1215,6 +1225,11 @@ void StraightForwardRegisterAllocator::AssignFixedTemporaries(NodeBase* node) {
     }
     general_registers_.block(reg);
   }
+
+  if (FLAG_trace_maglev_regalloc) {
+    printing_visitor_->os()
+        << "Fixed temporaries: " << fixed_temporaries << "\n";
+  }
 }
 
 void StraightForwardRegisterAllocator::AssignArbitraryTemporaries(
@@ -1245,17 +1260,7 @@ void StraightForwardRegisterAllocator::AssignArbitraryTemporaries(
   DCHECK_GE(temporaries.Count(), node->num_temporaries_needed());
   node->assign_temporaries(temporaries);
   if (FLAG_trace_maglev_regalloc) {
-    printing_visitor_->os() << "Temporaries: ";
-    bool first = true;
-    for (Register reg : temporaries) {
-      if (first) {
-        first = false;
-      } else {
-        printing_visitor_->os() << ", ";
-      }
-      printing_visitor_->os() << reg;
-    }
-    printing_visitor_->os() << "\n";
+    printing_visitor_->os() << "Temporaries: " << temporaries << "\n";
   }
 }
 

src/maglev/maglev-vreg-allocator.h

@@ -26,27 +26,7 @@ class MaglevVregAllocationState {
 
 class MaglevVregAllocator {
  public:
-  void PreProcessGraph(MaglevCompilationInfo*, Graph* graph) {
-    for (Constant* constant : graph->constants()) {
-      constant->AllocateVreg(&state_);
-    }
-    for (const auto& [index, constant] : graph->root()) {
-      constant->AllocateVreg(&state_);
-      USE(index);
-    }
-    for (const auto& [index, constant] : graph->smi()) {
-      constant->AllocateVreg(&state_);
-      USE(index);
-    }
-    for (const auto& [index, constant] : graph->int32()) {
-      constant->AllocateVreg(&state_);
-      USE(index);
-    }
-    for (const auto& [index, constant] : graph->float64()) {
-      constant->AllocateVreg(&state_);
-      USE(index);
-    }
-  }
+  void PreProcessGraph(MaglevCompilationInfo*, Graph* graph) {}
   void PostProcessGraph(MaglevCompilationInfo*, Graph* graph) {
     for (BasicBlock* block : *graph) {
       if (!block->has_phi()) continue;