[llvm] [MachinePipeliner] Fix constraints aren't considered in certain cases (PR #95356)

[via llvm-commits]

================
@@ -2420,47 +2420,48 @@ bool SwingSchedulerDAG::schedulePipeline(SMSchedule &Schedule) {
       // upon the scheduled time for any predecessors/successors.
       int EarlyStart = INT_MIN;
       int LateStart = INT_MAX;
-      // These values are set when the size of the schedule window is limited
-      // due to chain dependences.
-      int SchedEnd = INT_MAX;
-      int SchedStart = INT_MIN;
-      Schedule.computeStart(SU, &EarlyStart, &LateStart, &SchedEnd, &SchedStart,
-                            II, this);
+      Schedule.computeStart(SU, &EarlyStart, &LateStart, II, this);
       LLVM_DEBUG({
         dbgs() << "\n";
         dbgs() << "Inst (" << SU->NodeNum << ") ";
         SU->getInstr()->dump();
         dbgs() << "\n";
       });
-      LLVM_DEBUG({
-        dbgs() << format("\tes: %8x ls: %8x me: %8x ms: %8x\n", EarlyStart,
-                         LateStart, SchedEnd, SchedStart);
-      });
+      LLVM_DEBUG(
+          dbgs() << format("\tes: %8x ls: %8x\n", EarlyStart, LateStart));
 
-      if (EarlyStart > LateStart || SchedEnd < EarlyStart ||
-          SchedStart > LateStart)
+      if (EarlyStart > LateStart)
         scheduleFound = false;
-      else if (EarlyStart != INT_MIN && LateStart == INT_MAX) {
-        SchedEnd = std::min(SchedEnd, EarlyStart + (int)II - 1);
-        scheduleFound = Schedule.insert(SU, EarlyStart, SchedEnd, II);
-      } else if (EarlyStart == INT_MIN && LateStart != INT_MAX) {
-        SchedStart = std::max(SchedStart, LateStart - (int)II + 1);
-        scheduleFound = Schedule.insert(SU, LateStart, SchedStart, II);
-      } else if (EarlyStart != INT_MIN && LateStart != INT_MAX) {
-        SchedEnd =
-            std::min(SchedEnd, std::min(LateStart, EarlyStart + (int)II - 1));
-        // When scheduling a Phi it is better to start at the late cycle and go
-        // backwards. The default order may insert the Phi too far away from
-        // its first dependence.
-        if (SU->getInstr()->isPHI())
-          scheduleFound = Schedule.insert(SU, SchedEnd, EarlyStart, II);
-        else
-          scheduleFound = Schedule.insert(SU, EarlyStart, SchedEnd, II);
+      else if (EarlyStart != INT_MIN && LateStart == INT_MAX)
+        scheduleFound =
+            Schedule.insert(SU, EarlyStart, EarlyStart + (int)II - 1, II);
+      else if (EarlyStart == INT_MIN && LateStart != INT_MAX)
+        scheduleFound =
+            Schedule.insert(SU, LateStart, LateStart - (int)II + 1, II);
+      else if (EarlyStart != INT_MIN && LateStart != INT_MAX) {
+        // To keep original behaviour, start scheduling at the late cycle and go
+        // backwards when all scheduled predecessors are loop-carried
+        // output/order dependencies. Empirically, there are also cases where
+        // scheduling becomes possible with backward search.
+        if (Schedule.onlyHasLoopCarriedOutputOrOrderPreds(SU, this)) {
----------------
bcahoon wrote:

I commented out the code between lines 2446-2449, and noticed some test failures. I took a quick look at sw-bad-sched.ll, and I can see what is going on.

Prior to this patch:
  ```
Inst (3)   %63:intregs = L2_loadri_io %15:intregs, 0 :: (load (s32) from %ir.lsr.iv1, !tbaa !0)
        es: 80000000 ls:        0 me: 7fffffff ms: ffffffff
  Trying to insert node between 0 and -1 II: 3
```

With this patch:
  ```
Inst (3)   %63:intregs = L2_loadri_io %15:intregs, 0 :: (load (s32) from %ir.lsr.iv1, !tbaa !0)
        es: ffffffff ls:        0
  Trying to insert node between 0 and -1 II: 3

```
In the old case, the pipeliner uses does a bottom up schedule because es == INT_MAX and ls == 0.
Because es is -1, with the patch, the pipeliner drops into the case when both es and ls have been defined. Previously, that used a top-down search (unless the SU is a Phi).

I think it's okay for the schedule processing to change after your patch. I feel it would be surprising if it didn't.  For example, in the case of swp-bad-sched.ll, the generated schedules are equivalent when comparing the output from your patch to the output from removing the code in lines 2446-2449. I haven't looked at the other cases though.

The question is - does the bottom-up scheduling produces a better result for certain situations. The patch has a comment "Empirically, there are also cases where scheduling becomes possible with backward search". This would be a reason to add/keep the code that does a backward search. But, I would see those cases and I think it would be better to combine any changes to do a bottom up search with the isPHI() check


https://github.com/llvm/llvm-project/pull/95356