[PATCH] D17260: SystemZ scheduling implementation

[Andrew Trick via llvm-commits]

atrick accepted this revision.
atrick added a comment.
This revision is now accepted and ready to land.

Overall nice job. I won't be able to review your hazard recognizer or any of the SystemZ models.


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:123-125
@@ -118,6 +122,5 @@
 
-  // TODO: It may be better to default to latency-oriented scheduling, however
-  // LLVM's current latency-oriented scheduler can't handle physreg definitions
-  // such as SystemZ has with CC, so set this to the register-pressure
-  // scheduler, because it can.
-  setSchedulingPreference(Sched::RegPressure);
+  // XXJ Experimental.
+  if (Subtarget.isZ10())
+    setSchedulingPreference(Sched::RegPressure);
+  else {
----------------
You won't be able to rely on this since SelectionDAG scheduler is deprecated. It's just waiting for a replacement. I think you should focus on the right MI scheduler heuristics for your target.

That said, I can see why you did this because your MI scheduler is top-down only so it might be hard for you to control register pressure. As an alternative, we could easily support a two-pass MI scheuler, bottom-up, then top-down.

================
Comment at: lib/Target/SystemZ/SystemZInstrInfo.h:156-158
@@ -153,1 +155,5 @@
                         DebugLoc DL) const override;
+  ScheduleHazardRecognizer*
+  CreateTargetMIHazardRecognizer(const InstrItineraryData*,
+                                 const ScheduleDAG *DAG) const override;
+
----------------
FYI, the "new" machine model is meant to be flexible enough that you don't need to create your own hazard recognizer (you can add predicates and arbitrary pseudo machine resources). However, it's tricky to do that and fine just to use a hazard recognizer when you have complicated decode/issue group constraints.

================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:43
@@ +42,3 @@
+def Z13_VFUnit  : ProcResource<2> { let BufferSize = 2; /* ooo */ }
+def Z13_FPdUnit : ProcResource<2> { let BufferSize = 0; /* blocking */ }
+
----------------
In-order scheduling with multiple functional units of the same type is somewhat broken in the generic scheduler. ReservedCycles is only tracking a worst-cast resource availability across all units. It should really be a two-dimensional array. I know this has been fixed out-of-tree at least one target but never pushed back. It would be great if you fixed that!


http://reviews.llvm.org/D17260