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

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne created this revision.
JamesNagurne added a reviewer: jmolloy.
Herald added a project: All.
JamesNagurne requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

https://discourse.llvm.org/t/incorrect-recmii-calculations-in-machinepipeliner/63531

The pipeliner's calculation of RecMII depends on the 'Latency' calculated
from each elementary circuit.

Consider the follow example: a DAG with a triange dependence of the form:

  1 -> 2, 1 -> 3, 2 -> 3, and 3 -> 1

Assume edges are all weighted 1.

The current calculation for NodeSet Latency checks only that the successor
is in the circuit, and not that the successor is part of the path taken by
the particular elementary circuit.

This presents as the following with the given DAG:

- Circuit 1 -> 3 correctly calculated as Latency = 1
- Circuit 1 -> 2 -> 3 incorrectly calculated as Latency = 3 and not 2

The solution here notes that Johnson's algorithm returns the elementary
circuit in the order that the path is taken. This fact is used to ensure
that only edges to the immediate successor in the circuit is checked for
latency.

I've added a common-thread reviewer for a few past commits, but welcome others to add and review themselves.
The subscribers I've added are colleagues as well as past MachinePipeliner committers as interested parties.

As an aside: Are discourse links static and persistent? Or should I remove it from the log here and only reference it in the review?


Repository:
  rG LLVM Github Monorepo

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
@@ -334,20 +334,21 @@
   NodeSet(iterator S, iterator E) : Nodes(S, E), HasRecurrence(true) {
     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.442386.patch
Type: text/x-patch
Size: 1465 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220705/11bea0d2/attachment.bin>