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

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

thopre 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)
----------------
dpenry wrote:
> 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.
> 
> 
Ah ok, thanks for the explanation.


================
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.
----------------
dpenry wrote:
> 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.
Ok


================
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;
----------------
dpenry wrote:
> 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.
> 
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122672