[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:
> > 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.
While running in BottomUp, the predecessors are being looked at, and the branch (which is ExitSU) isn't a predecessor and thus there's no "falingl off" onto a boundaryNode in predecessors, at least for the kinds of branch instructions we've seen so far.

The anti-dependence processing is definitely needed in some cases.  If we dropped the Artificial check in ignoreDependence and replaced it with a test for isBoundaryNode, I think that has a good chance of working.

It would also remove some interesting "failure to pipeline" cases that I've seen when scheduling takes into account issue width and copy pseudo-ops have non-zero costs.  That's another patch I'm working on, but haven't uploaded yet.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124267