[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 446599.
JamesNagurne marked an inline comment as done.
JamesNagurne added a comment.

Apologies for "one last" revision, but I wanted to make sure the review reflected my intended commit.

I've incorporated the "in-spirit" modulo operation, as well as a check to ensure the circuit path is in order.

I think there is some very good merit in calculating Latency as part of the ::circuit algorithm, but did not want to bite that off just yet.


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,38 @@
 
   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 = Nodes[(i + 1) % e];
+
+      // Calculate the maximum latency to NextSUnit from Nodes[i]
+      const SDep *MaxEdge = nullptr;
       for (const SDep &Succ : Nodes[i]->Succs) {
-        auto SuccSUnit = Succ.getSUnit();
-        if (!Nodes.count(SuccSUnit))
+        SUnit *SuccSUnit = Succ.getSUnit();
+        // Skip edges that are not part of this circuit
+        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 ((MaxEdge == nullptr) || (CurLatency > MaxEdge->getLatency()))
+          MaxEdge = &Succ;
       }
-      for (auto SUnitLatency : SuccSUnitLatency)
-        Latency += SUnitLatency.second;
+      assert(MaxEdge != nullptr && "Expected path successor in circuit");
+      Latency += MaxEdge->getLatency();
     }
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129161.446599.patch
Type: text/x-patch
Size: 2215 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220721/4c4bdad7/attachment.bin>