[PATCH] D17260: SystemZ scheduling implementation

[Jonas Paulsson via llvm-commits]

jonpa requested a review of this revision.
jonpa added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.cpp:154
+
+    // trim e.g. Z13_FXaUnit -> FXa
+    if (U.find("FXa") != std::string::npos)
----------------
uweigand wrote:
> This looks a bit ad-hoc ... isn't there a more generic way to find the shorter name?
Fixed by using string::substr()/resize() instead. Now all units should really be named per a Z13_XXXUnit pattern.


================
Comment at: lib/Target/SystemZ/SystemZMachineScheduler.h:57
+  struct SUSorter {
+    static ScheduleDAGMI *DAG;
+
----------------
uweigand wrote:
> 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 ...
I gave this a try, but it thought it was a bit messy to flip the sorting variables back and forth inside a set of Candidates.

I instead use the isScheduleHigh flag for groupers / FPd ops, which simplifies the SUSorter method while also eliminating the static variable. The iteration in pickNode() should be nearly unaffected, since these nodes are quite rare.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:27
+    // This model does not include operand specific information.
+    let CompleteModel = 0;
+}
----------------
uweigand wrote:
> 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?
I added the hasNoSchedulingInfo flag on the appropriate instructions, and then set CompleteModel (the reason I did not do that before is that AFAICR, this flag then also demanded modeling of operand writes or something of that sort).

Worthy of mentioning is that this triggers an error during build for any instruction missing scheduling input for *all* subtargets. In a debug build, TargetSchedule.cpp will then cause an abort during compilation if the subtarget does not have scheduling input for an emitted instruction.



================
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.
+
----------------
uweigand wrote:
> Ideally, the ordering in these files should mostly correspond to the ordering in the original InstrInfo files, just to make them easier to find ...
I have reorganized the files completely to match the InstrInfo file sections.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:104
+// resources that it needs. These will be combined into a SchedClass.
+
+//  Call
----------------
uweigand wrote:
> 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 ...
Could not as of yet find any way to achieve this, but there might be some way...


https://reviews.llvm.org/D17260