[PATCH] D124267: [MachinePipeliner] Fix unscheduled instruction

[David Penry via Phabricator via llvm-commits]

dpenry added inline comments.


================
Comment at: llvm/lib/CodeGen/MachinePipeliner.cpp:1582
   for (auto &PI : Cur->Preds)
-    if (PI.getKind() == SDep::Anti)
+    if (PI.getKind() == SDep::Anti && !ignoreDependence(PI, true))
       FoundPath |=
----------------
thopre wrote:
> bcahoon wrote:
> > I don't think this if condition will ever be true?  If PI.getKind() == SDep::Anti, then ignoreDepedendence(PI, true) will also return true, so this condition evaluates to if (true && false).
> Mmmh indeed. I did this because pred_L calls ignoreDependence so I wanted to be consistent. On the other hand computeNodeOrder does not call ignoreDependence in the BottomUp case. What's the reason for the inconsistency?
I've been looking at the artificial edges of late...

I suspect the inconsistency in computeNodeOrder (a check which doesn't figure into the algorithm in the literature) is there because ignoreDependence is being used beyond its stated purpose of preventing unbounded recursion when computing ALAP/ASAP/etc.

It is now also being used in computeNodeOrder, pred_L, and succ_L for its D.isArtificial() test, which is guarding against following an edge to the ExitSU (branch) node.  This edge is an artificial edge for the machine-loop-branch terminated blocks that have been pipelined so far.  I found for https://reviews.llvm.org/D122672 that a test for D.getSUnit()->isBoundaryNode() was also necessary -- with general branches you can have non-artificial edges to the branch -- in this function.

It may be the case that ignoreDependence shouldn't have a test for isArtificial or isBoundaryNode at all, but that computeNodeOrder and succL should check isBoundaryNode and pred_L should check  isBoundaryNode  and D.getKind() == Anti.  Indeed, there may be other places where isArtificial is tested that D122672 adds isBoundaryNode tests that don't need isArtificial.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124267