[PATCH] D17260: SystemZ scheduling implementation

Jonas Paulsson via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 19 04:39:41 PDT 2016


jonpa added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:27
+    // This model does not include operand specific information.
+    let CompleteModel = 0;
+}
----------------
uweigand wrote:
> 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 ...
I found that my previous statement on the compile time checking for scheduling input was not actually correct - this does not really cover all instructions:

Currently asserts trigger only if
* All subtargets have omitted the instruction from Schedule .td files. TableGen catches this, and only this because it isn't clever enough to know if a given subtarget (with a missing sched class for an instruction), actually supports that instruction or not.
* An instruction has a def operand and the sched class does not have a WriteLatency entry for it. This is a specialized assert (in computeOperandLatency()) which doesn't cover all instructions - I could for instance remove the InstRW for a compare and not get any assert triggering.

So there really isn't any general assert that checks that for a subtarget with a CompleteModel, all instructions actually emitted have a sane scheduling class. What happened if I removed the InstRW for an instruction, was that it got its own (valid) schedclass for the subtarget, but with just 0 values.

I think we could catch the error of forgetting a subtarget/instruction sched annotation with an assert in the scheduler that demands at least one uop for any target instruction. This has at least worked well during my experiments previously.

Should I add this just in the SystemZ backend? Or could it be part of the common code somewhere? (I am thinking this should be done both pre-ra and post-ra). Or is there any reason not to demand this that I have missed?




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


================
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; }
+
----------------
uweigand wrote:
> What's the EC12 doing here?
Good heavens!


https://reviews.llvm.org/D17260





More information about the llvm-commits mailing list