[PATCH] D17260: SystemZ scheduling implementation

[Ulrich Weigand via llvm-commits]

uweigand added a comment.

In https://reviews.llvm.org/D17260#572785, @jonpa wrote:

> Updated per requests.
>
> Does anyone know how to say "Instruction B should have the same scheduling class as instruction A" ? This was the only point I could not get fixed.
>
> For the rest, please see replies to comments below.


Looking quite good now, thanks!   The one thing I'm still wondering about is the hasNoSchedulingInfo flags.  Those are of course fine for the .insn directive, and also for custom-inserter pseudos.  However, I don't think we want them for the Asm* branch variants; those are just regular branch instructions that might be used in inline assembler, and they really should have the same scheduling info as the standard form of the branch instructions.  I think it should be straightforward to implement this by adding "(Asm)?" to the instregex strings for the branch instructions.



================
Comment at: lib/Target/SystemZ/SystemZMachineScheduler.h:57
+  struct SUSorter {
+    static ScheduleDAGMI *DAG;
+
----------------
jonpa wrote:
> 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.
OK, that makes sense.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:27
+    // This model does not include operand specific information.
+    let CompleteModel = 0;
+}
----------------
jonpa wrote:
> 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.
> 
Great, thanks!  I do think we indeed want the error, even for older subtargets.  Hopefully in the not-too-distant future we'll have completed the instruction set for the old processors anyway ...


================
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.
+
----------------
jonpa wrote:
> 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.
Excellent, it is much more readable now!


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:104
+// resources that it needs. These will be combined into a SchedClass.
+
+//  Call
----------------
jonpa wrote:
> 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...
Oh well, if there's no straightforward way, it's OK the way it is now ...


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:26
+
+    let CompleteModel = 1;
+}
----------------
I think this is the default setting, so we can just leave it out here.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ196.td:67
+def : WriteRes<LSU_lat1,  [Z196_LSUnit]> { let Latency = 1; }
+def : WriteRes<FPU,      [ZEC12_FPUnit]> { let Latency = 8; }
+
----------------
What's the EC12 doing here?


https://reviews.llvm.org/D17260