[PATCH] D122672: [CodeGen][ARM] Enable Swing Module Scheduling for ARM

[David Penry via Phabricator via llvm-commits]

dpenry marked an inline comment as done.
dpenry added inline comments.


================
Comment at: llvm/lib/CodeGen/MachinePipeliner.cpp:1796
     SUnit *Successor = SI.getSUnit();
-    if (!SI.isArtificial() && NodesAdded.count(Successor) == 0)
+    if (!SI.isArtificial() && !Successor->isBoundaryNode() &&
+        NodesAdded.count(Successor) == 0)
----------------
thopre wrote:
> Should we use ignoreDependence here?
Even if we were to change ignoreDependence to be just a test for boundary node and a guarded test for anti-dependence, I think we shouldn't call it here.  We're not trying to ignore a dependence here, but rather, we're trying to avoid putting the ExitSU node into the node set, and directly testing for that is more clear.  If I'm reading the history of the code right, the artificial test was originally an attempt to do that and then the addition of more artificial edges due to the CopyToPHI DAG mutation obscured what is going on. 

I'd prefer to sort out "where is only a boundary node check" and "where is ignoreDependence" and "can we drop artificial edge checks"  in another patch, in part because I was hoping that this particular patch wouldn't affect Hexagon or PowerPC.




================
Comment at: llvm/lib/CodeGen/MachinePipeliner.cpp:1802
     SUnit *Predecessor = PI.getSUnit();
     if (!PI.isArtificial() && NodesAdded.count(Predecessor) == 0)
       addConnectedNodes(Predecessor, NewSet, NodesAdded);
----------------
thopre wrote:
> Sorry for the naive question but why are anti-dependences ok here?
No problem.  The more people that understand the code, the better off we'll be!

At any rate, the original SMS paper wants to add all connected nodes here, including those connected by anti-dependences.

"E is the set of edges, where each edge (u,v) Î E
represents a dependence from operation u to operation
v. Only data dependences (flow, anti and output-dependences)
are included ..."

"Finally, the remaining nodes are
grouped into sets of the same priority but this priority is
lower than that of the sets containing recurrences. Each one
of these sets consists of the nodes of a connected
component of the graph that do not belong to any previous
set."



================
Comment at: llvm/lib/CodeGen/MachinePipeliner.cpp:2272
 /// Return true for an order or output dependence that is loop carried
 /// potentially. A dependence is loop carried if the destination defines a valu
 /// that may be used or defined by the source in a subsequent iteration.
----------------
thopre wrote:
> Maybe fix the typo while you're at it
I had some typo fixes, but was requested to move them to a separate patch.


================
Comment at: llvm/lib/CodeGen/MachinePipeliner.cpp:2277
   if ((Dep.getKind() != SDep::Order && Dep.getKind() != SDep::Output) ||
-      Dep.isArtificial())
+      Dep.isArtificial() || Dep.getSUnit()->isBoundaryNode())
     return false;
----------------
thopre wrote:
> Can we drop Dep.isArtificial()?
I expect we could, but as I mentioned above, I'd prefer to clean up the extra artificial checks in a separate patch.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122672