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

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachinePipeliner.h:339
+      // the first node
+      SUnit *NextSUnit = ((i + 1) == e) ? Nodes[0] : Nodes[i + 1];
+
----------------
JamesNagurne wrote:
> 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
I couldn't find any specific "It returns this order", but there are a number of hints:

I'm using only a copy of the paper I found at https://www.cs.tufts.edu/comp/150GA/homeworks/hw1/Johnson%2075.PDF

First, definitions:


  - A path is a defined as a set of directed jumps from one node to an adjacent node in a graph
  - A circuit is a path whose start and end nodes are the same
  - An elementary circuit is a circuit which never jumps from the same node twice.
  - Johnson's circuit algorithm finds elementary circuits

The algorithm uses the concept of a 'root vertex s', which is defined as a least vertex through some definition of lesser/greater. In the code, this ordering is defined by SUnit's NodeNum field, which is a good approximation for ordering.

The algorithm then repeatedly appends to the path from s to some adjacent node n, and then uses n as the starting point for the next iteration of the algorithm. Since each step jumps from a previously selected, adjacent node to the next, I don't see how it could be out of order. This is further reinforced because each node is guaranteed to be "greater" than the last.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129161