[PATCH] D150312: [MISched] Introduce and use ResourceSegments.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:96
 #include <memory>
+#include <ostream>
 #include <string>
----------------
#include "llvm/Support/raw_ostream.h" ?


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:716
+      for (auto Interval : _Intervals)
+        assert(!intersects(A, Interval) && "A resource is being overwritten");
+#endif
----------------
Use assert(all_of(_Intervals))


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:815
+    ///       [C-Cycle+1, C-StartAtCycle+1)
+    static std::pair<long, long>
+    getResourceIntervalBottom(unsigned C, unsigned StartAtCycle,
----------------
typedef std::pair<long, long> ? in fact we might be better off using a std::pair<int64_t,int64_t> ?


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:877
+        std::function<std::pair<long, long>(unsigned, unsigned, unsigned)>
+            IntervalBuilder) const {
+      assert(is_sorted() && "Cannot execute on an un-sorted set of intervals.");
----------------
Some of these larger implementations might be better off in MachineScheduler.cpp 


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:880
+      unsigned RetCycle = CurrCycle;
+      auto NewInterval = IntervalBuilder(RetCycle, StartAtCycle, Cycle);
+      for (auto &Interval : _Intervals) {
----------------
(style) avoid auto


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:887
+        // intersects.
+        assert(Interval.second > NewInterval.first);
+        RetCycle += (unsigned)Interval.second - (unsigned)NewInterval.first;
----------------
(style) assertion message


================
Comment at: llvm/include/llvm/MC/MCSchedule.h:308
+  // `SchedMachineModel` in
+  // llvm/include/llvm/Target/TargetSchedule.td.
+  bool EnableIntervals;
----------------
Include the description here as well - don't just refer to TargetSchedule.td


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:170
+    MIResourceCutOff("misched-resource-cutoff", cl::Hidden,
+                     cl::desc("Number of intervals to track"), cl::init(10));
+
----------------
Test coverage?


================
Comment at: llvm/unittests/CodeGen/CMakeLists.txt:41
   MLRegallocDevelopmentFeatures.cpp
+  SchedBoundary.cpp
   )
----------------
These are supposed to be kept sorted :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150312