[PATCH] D124267: [MachinePipeliner] Fix unscheduled instruction

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked an inline comment as not done.
thopre 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 |=
----------------
bcahoon wrote:
> dpenry wrote:
> > 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.
> > 
> Good question. I don't recall why ignoreDependence is not called in the BottomUp case. Seems like it should be there. And, yes, it may be clearer to change up ignoreDependence. Part of the confusion is because we need special processing of the anti-depenences that represent back-edges.
We also added a few isBoundaryNode check in our backend so I'm looking forward for your patch to land. Why do you want to put the isBoundaryNode check outside ignoreDependence? From your comment it seems it would make more sense to just replace the isArtificial check by a boundary node check and make sure ignoreDependence is used consistently. I'm happy to rework the patch accordingly if you agree with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124267