[PATCH] D17260: SystemZ scheduling implementation

[Jonas Paulsson via llvm-commits]

jonpa added inline comments.

================
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 {
----------------
atrick wrote:
> 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.
I would hope that the pre-ra MI scheduler is bidirectional, due to the overrideSchedPolicy() call, where this is selected. Is there a better /normal way of doing this, perhaps?



================
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 */ }
+
----------------
atrick wrote:
> 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!
This unit is handled as a special case by the HazardRecognizer, I guess partly because I couldn't see that the code - as you say - did what I wanted.

That would be interesting to fix... could perhaps this out-of-tree target push this possibly?




http://reviews.llvm.org/D17260