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

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne added a comment.

In D129161#3661616 <https://reviews.llvm.org/D129161#3661616>, @barannikov88 wrote:

>> Circuit 1 -> 2 -> 3 incorrectly calculated as Latency = 3 and not 2
>
> Why should it be 2? There are three edges on the critical path (1 -> 2, 2 -> 3 and 3 -> 1), so the latency is 3.

As I said in the previous comment, I could be wrong, but I understood the algorithm as not properly calculating the latency of the edge from node 3 to the PHI at the top of the loop, so I did not add that value. Regardless, if that edge is considered, and is given a value of 1, the calculation in the currently upstreamed code will give a latency of 4, not 3, due to the latency of edge 1 -> 3 being erroneously considered.



================
Comment at: llvm/include/llvm/CodeGen/MachinePipeliner.h:339
+      // the first node
+      SUnit *NextSUnit = ((i + 1) == e) ? Nodes[0] : Nodes[i + 1];
+
----------------
hgreving wrote:
> This relies on "notes that Johnson's algorithm returns the elementary circuit in the order that the path is taken". Could you add this as a comment here, maybe even add a note at line 321 to SetVector Node, that Node needs to be ordered (and the SetVector isn't there just to avoid non-determinism).
> 
> Can you also add a pointer to the fact that it does reutrn the path in order (I myself for example didn't know this).
I'll do


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129161