[PATCH] D36704: [CodeGen] Improve the consistency of instruction fusion

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:38-46
+  // Check that neither instr is already paired with another along the edge
+  // between them.
+  for (SDep &SI : FirstSU.Succs)
+    if (SI.isCluster())
+      return false;
+
+  for (SDep &SI : SecondSU.Preds)
----------------
evandro wrote:
> MatzeB wrote:
> > evandro wrote:
> > > MatzeB wrote:
> > > > Given the code below that adds all succs of first as succs of second, and vice versa for preds of seconds. How can the situation you are checking here even happen without us failing the cycle check in addEdge anyway?
> > > > 
> > > It can happen that an instr can be eligible for fusion for more than one other instruction.  This check limits the fusion to the first seen eligible one.
> > Hmm ok I was thinking of this:
> > ```
> >    A
> >   / \
> >  B   C
> > ```
> > If we see A+B first and fuse it the loop further down will add an extra edge between B and C:
> > ```
> >   A
> >  / \
> > B  |
> >  \ |
> >    C
> > ```
> > at this point fusing A+C is pointless because we always must schedule B in between. I was wrong however in thinking that the following addEdge would fail because of this. Nonetheless it would be good to have a check for this situation to avoid adding cluster edges for pairs that cannot be scheduled next to each other anyway. Maybe we should have reachability checks from FirstSU successors to SecondSU and bail if one is found.
> What do you mean by reachability exactly?  Also, do you think that `addEdge()` should have this check added?
Having a path between the two SDNodes, there is `Topo.IsReachable(A, B)`. It is used by addEdge, canAddEdge already to avoid situations in which we would form cycles which would be plain invalid.
This case however would not be an invalid ScheduleDAG, just one where we would know beforehand that we cannot fulfill the cluster dependency. So this should be checked outside of `addEdge()` IMO.


https://reviews.llvm.org/D36704