[PATCH] D17260: SystemZ scheduling implementation

[Ulrich Weigand via llvm-commits]

uweigand added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.cpp:44
+    static_cast<const SystemZSubtarget&>(C->MF->getSubtarget());
+  TII = static_cast<const SystemZInstrInfo *>(ST.getInstrInfo());
+
----------------
I guess instead of this we could simply use getInstrInfo on the SchedModel.


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.cpp:84
+
+void SystemZHazardRecognizer::init() {
+  CurrGroupSize = 0;
----------------
Is there a reason why this is a separate function and no just done directly in ::Reset()?


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.cpp:154
+
+    // trim e.g. Z13_FXaUnit -> FXa
+    if (U.find("FXa") != std::string::npos)
----------------
This looks a bit ad-hoc ... isn't there a more generic way to find the shorter name?


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.h:120
+  /// means it would be good to schedule SU next.
+  void resourcesCost(SUnit *SU, int &Cost);
+
----------------
Why does groupingCost use the return value while resourcesCost uses an output parameter?


================
Comment at: lib/Target/SystemZ/SystemZMachineScheduler.h:57
+  struct SUSorter {
+    static ScheduleDAGMI *DAG;
+
----------------
As discussed offline, we really need to get rid of the gobal/static variable here.   I note that there is quite a bit of similarily between this "preliminary" sorter and the final sorter in Candidate.   Maybe we should actually store "preliminary" candidates in the Available list (with GroupingCost and ResourceCost only set to 0 or 1 depending on whether grouping or reserved resources are involved), and the update the cost parameters with actual values once we known them?   We then might even be able to reuse the same comparison routine ...


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:27
+    // This model does not include operand specific information.
+    let CompleteModel = 0;
+}
----------------
We should really try to get this complete, so that instructions added in the future don't accidentally lack scheduling information.  How difficult would it be to get there?


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:103
+// For each instruction, as matched by a regexp, provide a list of
+// resources that it needs. These will be combined into a SchedClass.
+
----------------
Ideally, the ordering in these files should mostly correspond to the ordering in the original InstrInfo files, just to make them easier to find ...


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:104
+// resources that it needs. These will be combined into a SchedClass.
+
+//  Call
----------------
Also, it is somewhat annoying that we need to list not just the basic instruction definitions, but all the various aliases as well.  I'm wondering if there isn't some what to annotate the Alias definition in the main file with the opcode the alias will be resolved in the end, so that can be used for scheduling purposes ...


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:224
+// And
+def : InstRW<[FXb, LSU, Lat5], (instregex "NTSTG$")>;
+def : InstRW<[FXa, LSU, Lat5], (instregex "N(G|Y)?$")>;
----------------
This is not an "And", it's a non-transactional store and should go with the transaction-related instructions.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:581
+
+// Various
+def : InstRW<[VecXsPm], (instregex "VA(B|F|G|H|Q|CQ)$")>;
----------------
It would be nice to at least separate out vector floating-point instructions, so we can easily see where W variants are needed.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:738
+
+///// INLINE ASSEMBLY
+
----------------
I don't think there's a real difference between those and the ones listed under Other.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:763
+// Load the Global Offset Table address
+def : InstRW<[FXa], (instregex "GOT$")>;
+
----------------
This is just an alias for a LARL and should go there.


https://reviews.llvm.org/D17260