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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachinePipeliner.h:359
           continue;
         unsigned CurLatency = Succ.getLatency();
+        if (CurLatency > SuccLatency)
----------------
JamesNagurne wrote:
> 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.
@JamesNagurne 

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

In case that helps:
```
LoopBB:
  %1 = PHI %2, LoopBB, ...
  %i = PHI %i, LoopBB, %i_init, PHBB
       ST %1, [%ptr + %i + 1]
  %2 = LD [%ptr + %i]
  %i = ADD %i, 1
  ```

In the initial DAG (built by buildSchedGraph) there is Anti dependence LD -> PHI (PHI depends on LD) and Data dependence ST -> PHI (ST depends on PHI).
Note that the direction of dependency between LD and PHI is reversed. This is due to the DAG nature - it cannot contain cycles, because, well, 'A' stands for Acyclic.

Note that LD and ST access different memory locations, so there are independent of each other. Also note that the store uses the value defined by the load on the previous iteration. From the point of view, they //are// dependent. This dependency is called "loop-carried".

The loop-carried dependency does not exist in the initial DAG as it is not needed for the main user of buildSchedGraph -- MachineScheduler -- it can freely reorder these operations because there are still virtual registers. The worst case scenario here is that register allocator will have to insert a COPY instruction due to overlapping lifetimes of %1 and %2.

But this kind of dependence is important for pipeliner for reasons I don't clearly remember. So it adds an edge between the load and the store so that they are not reordered, but only if it could not prove that the two operations from the current and the next iteration do not alias (in the above example they do).

The "chain" here has exact same meaning as in SelectionDAG -- it enforces order between two nodes which are otherwise independent; it effectively is an "order" dependence, with "scheduling barrier" flavor (see enum OrderKind).

If you are interested, I could try to recall why adding this edge is necessary.



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

https://reviews.llvm.org/D129161