[PATCH] D128941: [ModuloSchedule] Add interface call to accept/reject SMS schedules

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

At a high level shouldUseSchedule sounds like a useful addition, but much of the logic in the current Arm implementation doesn't seem very Arm specific (correct me if I'm wrong). Would it be beneficial to incorporate the register pressure calculations into SwingSchedulerDAG, which the target could opt into checking.



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:738
 
+    /// Return true if the proposed schedule should used.  Otherwise return
+    /// false. This function can be used to ensure that pipelined loops meet
----------------
"should be used"
Perhaps explain that returning false means no pipelined loops too. "Otherwise return false to not pipeline the loop."


================
Comment at: llvm/lib/CodeGen/MachinePipeliner.cpp:2098
+  if (scheduleFound)
+    scheduleFound = LoopPipelinerInfo->shouldUseSchedule(*this, Schedule);
+
----------------
Perhaps move this below the "Schedule Found?" message below, and adding a debug message explaining that the target opted out of the schedule might be useful for debugging.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:6842
+  for (const auto &N : CIN) {
+    int cnt = N.second.count() - N.second[SEEN_AS_LIVE] * 2;
+    for (int i = 0; i < cnt; ++i)
----------------
cnt -> Cnt


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:6843
+    int cnt = N.second.count() - N.second[SEEN_AS_LIVE] * 2;
+    for (int i = 0; i < cnt; ++i)
+      RPT.increaseRegPressure(Register(N.first), LaneBitmask::getNone(),
----------------
i -> I


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:6888
+  std::vector<SUnit *> ProposedSchedule;
+  for (int cycle = SMS.getFinalCycle(); cycle >= SMS.getFirstCycle(); --cycle)
+    for (int stage = 0, stageEnd = SMS.getMaxStageCount(); stage <= stageEnd;
----------------
cycle -> Cycle. Same for the rest of the variables,


================
Comment at: llvm/test/CodeGen/Thumb2/swp-regpressure.mir:6
+# schedule to be rejected and that a test with the same resource usage
+# but without register pressure is not rejected.
+
----------------
This test uses phi nodes to increase the register pressure? Maybe remove the dead phis from dot2.

And maybe rename the tests to make it clear that dot1 has too high pressure, but dot2 is OK and expected to pipeilne.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128941