[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:359
           continue;
         unsigned CurLatency = Succ.getLatency();
+        if (CurLatency > SuccLatency)
----------------
hgreving wrote:
> You could add an assertion that at least one of the successors should get here and we should never skip all successors, which would imply that sth with the nodes order is unexpected.
@hgreving Thank you for planting the seed that I should be checking my assumption more rigorously.
It is indeed the case that the algorithm doesn't find a backedge successor in a given elementary circuit.

The upstream pipeliner has '::addLoopCarriedDependences', which creates "chain edges" (which I can't find a good def. for) from a load to a store.

These edges are specifically called out by ::createAdjacencyStructure and added to the structure. This results in a potential elementary circuit:


```
Load -> Op -> Store
  \-------------^
```

Note that there is no edge from Store to Load, only a special Order-Barrier edge from Load to Store (see ::isLoopCarriedDep).

I now believe there are 2 errors:

  # The original, where the path isn't being followed and extra edges are being added to the Latency calculation. I've fixed this.
  # The just-uncovered error, where there exists some backedges in a node's predecessor list, which is not being checked at all

My suggestion is to commit upstream the fix for 1, and then table a fix for 2 until I have more time to look into it.
That is, I will no longer assert MaxEdge, and will instead add 0, as the algorithm would have before my update. I will make a comment detailing this analysis as well.


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

https://reviews.llvm.org/D129161