[PATCH] D17260: SystemZ scheduling implementation
Ulrich Weigand via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 19 06:21:09 PDT 2016
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;
> 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.
More information about the llvm-commits