[PATCH] D17260: SystemZ scheduling implementation

[Ulrich Weigand via llvm-commits]

uweigand added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.cpp:47
+  if (!SC->isValid())
+    return 0; // IMPLICIT_DEF / KILL -- will not make impact in output.
+
----------------
This looks OK to me.  However, with this change we should now give a scheduling class to the DirectiveInsn pesudo instruction -- these do emit some instruction, we just don't know which one, so it should probably be modeled as some "generic" instruction.


================
Comment at: lib/Target/SystemZ/SystemZHazardRecognizer.h:112
+
+  /// Set Cost to a positive value if it would be better to wait with
+  /// SU, in regards to processor resources usage. A negative value
----------------
Seems you forgot to update the comment when changing the code :-)


================
Comment at: lib/Target/SystemZ/SystemZScheduleZ13.td:27
+    // This model does not include operand specific information.
+    let CompleteModel = 0;
+}
----------------
jonpa wrote:
> 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?
> 
> 
I don't think doing it in the backend is the right place.  In the backend, you only see the instructions that the code being compiled happens to use; and when you do find an error there, there's not much you can do.

The right place does seem to be TableGen.  And in fact, I had interpreted the code in CodeGenSchedModels::checkCompleteness to do just that check.  If this doesn't work as expected, it probably ought to be fixed there.

But in any case this is a separate problem and shouldn't hold up this patch.


https://reviews.llvm.org/D17260