[PATCH] D129161: [MachinePipeliner] Consider only direct path successors when calculating circuit latency

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne updated this revision to Diff 445931.
JamesNagurne added a comment.

Added comment detailing the expectation of iterators 'S' and 'E' to the Latency calculations.

It's interesting to note that a NodeSet is manipulated quite often after construction, such as when accumulating instructions not part of a circuit. It's only when calculation Latency that the ordering of the circuit must hold.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129161/new/

https://reviews.llvm.org/D129161

Files:
  llvm/include/llvm/CodeGen/MachinePipeliner.h


Index: llvm/include/llvm/CodeGen/MachinePipeliner.h
===================================================================
--- llvm/include/llvm/CodeGen/MachinePipeliner.h
+++ llvm/include/llvm/CodeGen/MachinePipeliner.h
@@ -332,22 +332,35 @@
 
   NodeSet() = default;
   NodeSet(iterator S, iterator E) : Nodes(S, E), HasRecurrence(true) {
+    // The following Latency calculation Assumes that the NodeNum for the
+    // SUnits in path S -> ... -> E is monotonically increasing. This is
+    // guaranteed, for example, by Johnson's Circuit algorithm with an ordering
+    // defined by SUnit::NodeNum.
+    //
+    // Future calls to insert/remove nodes from this NodeSet may break the
+    // below calculation.
+    //
+    // See SwingSchedulerDAG::findCircuits.
+    //
+    // [1] Donald B. Johnson, Finding all the elementary circuits of a directed
+    //     graph, SIAM Journal on Computing, 1975.
     Latency = 0;
     for (unsigned i = 0, e = Nodes.size(); i < e; ++i) {
-      DenseMap<SUnit *, unsigned> SuccSUnitLatency;
+      // The next node is either the next node in the list, or the backedge to
+      // the first node
+      SUnit *NextSUnit = ((i + 1) == e) ? Nodes[0] : Nodes[i + 1];
+
+      // Calculate the maximum latency to NextSUnit from Nodes[i]
+      unsigned SuccLatency = 0;
       for (const SDep &Succ : Nodes[i]->Succs) {
-        auto SuccSUnit = Succ.getSUnit();
-        if (!Nodes.count(SuccSUnit))
+        SUnit *SuccSUnit = Succ.getSUnit();
+        if (SuccSUnit->NodeNum != NextSUnit->NodeNum)
           continue;
         unsigned CurLatency = Succ.getLatency();
-        unsigned MaxLatency = 0;
-        if (SuccSUnitLatency.count(SuccSUnit))
-          MaxLatency = SuccSUnitLatency[SuccSUnit];
-        if (CurLatency > MaxLatency)
-          SuccSUnitLatency[SuccSUnit] = CurLatency;
+        if (CurLatency > SuccLatency)
+          SuccLatency = CurLatency;
       }
-      for (auto SUnitLatency : SuccSUnitLatency)
-        Latency += SUnitLatency.second;
+      Latency += SuccLatency;
     }
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129161.445931.patch
Type: text/x-patch
Size: 2061 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220719/417ad039/attachment.bin>