[PATCH] D30626: MachineScheduler/ScheduleDAG: Add support for GetSubGraph

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: include/llvm/CodeGen/ScheduleDAG.h:718
 
+    /// GetSubGraph - Returns an array of SUs that are both in the successor
+    /// subtree of StartSU and in the predecessor subtree of TargetSU.
----------------
Do not repeat the function name in the doxygen comment.


================
Comment at: include/llvm/CodeGen/ScheduleDAG.h:723-724
+    /// StartSU, else it is true.
+    std::vector<int> GetSubGraph(const SUnit *StartSU, const SUnit *TargetSU,
+                                 bool &Success);
+
----------------
Use references instead of pointers for things that cannot be nullptr.


================
Comment at: lib/CodeGen/ScheduleDAG.cpp:579
+      // Edges to non-SUnits are allowed but ignored (e.g. ExitSU).
+      if (s >= Node2Index.size())
+        continue;
----------------
Better use SUnit::isBoundaryNode()


================
Comment at: lib/CodeGen/ScheduleDAG.cpp:587
+      if (!Visited.test(s) && Node2Index[s] < UpperBound)
+        WorkList.push_back(SU->Succs[I].getSUnit());
+    }
----------------
Isn't there a mismatch here? I would assume you either test+set visited at the beginning of the loop or you do both when inserting into the worklist.

Testing before insertion but only setting the visited flag when pulling out of the worklist can lead to situations in which elements are pulled out multiple times.


================
Comment at: lib/CodeGen/ScheduleDAG.cpp:611
+      // Edges to non-SUnits are allowed but ignored (e.g. EntrySU).
+      if (s >= Node2Index.size())
+        continue;
----------------
Better use SUnit::isBoundaryNode()


Repository:
  rL LLVM

https://reviews.llvm.org/D30626