Page MenuHomePhabricator

TableGen: Check scheduling models for completeness
ClosedPublic

Authored by MatzeB on Feb 29 2016, 5:55 PM.

Details

Summary

For now this is only enabled for subtargets which set CheckCompleteness = 1 in their scheduling model.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 49441.Feb 29 2016, 5:55 PM
MatzeB retitled this revision from to TableGen: Check scheduling models for completeness.
MatzeB updated this object.
MatzeB added a reviewer: atrick.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
atrick edited edge metadata.Mar 1 2016, 8:32 AM

This seems 100% redundant with CompleteModel but the inverse meaning. Why don't you correct the flag for incomplete targets and file bugs against them?

MatzeB updated this revision to Diff 49517.Mar 1 2016, 11:28 AM
MatzeB edited edge metadata.

New version which which does introduce the new CheckCompleteness flag anymore but uses the existing CompleteModel flag. Also sets CompleteModel = 0 on incomplete models (which is all of them).

atrick accepted this revision.Mar 1 2016, 11:30 AM
atrick edited edge metadata.

LGTM; Are they all really incomplete though once you add your hasNoSchedulingInfo flag?

This revision is now accepted and ready to land.Mar 1 2016, 11:30 AM

LGTM; Are they all really incomplete though once you add your hasNoSchedulingInfo flag?

Yes. My guess this is because

  1. We currently only test completeness at runtime in assert builds but I don't think we test all the -mcpu=XXX variants that much in assert builds.
  2. Things currently used as scheduling barriers are never queried (so don't fail on missing info)
  3. Some instructions are not supported on every CPU in a target and have to be explicitely marked as unsupported.

LGTM; Are they all really incomplete though once you add your hasNoSchedulingInfo flag?

Yes. My guess this is because

  1. We currently only test completeness at runtime in assert builds but I don't think we test all the -mcpu=XXX variants that much in assert builds.
  2. Things currently used as scheduling barriers are never queried (so don't fail on missing info)
  3. Some instructions are not supported on every CPU in a target and have to be explicitely marked as unsupported.

You can also look here for an example of what was missing in the Cyclone model: http://reviews.llvm.org/D17748

This revision was automatically updated to reflect the committed changes.
dsanders added inline comments.Mar 2 2016, 5:57 AM
llvm/trunk/lib/Target/Mips/MipsScheduleP5600.td
16

Hi,

I see you've changed our P5600 model to be incomplete but it is actually complete. We want the compiler to assert if it's asked to schedule an instruction that has no scheduling information since these instructions are not supposed to be possible on the P5600. This assertion will no longer occur with this change.

Static checking should be more robust long-term than relying on the dynamic checks with the assert. This however means that instructions not supported by a CPU should be explicitely marked as unsupported. I just pushed a commit which lets you mark InstRW expressions as unsupported (on top of the existing support to mark scheduling resources as unsupported or whole instructions as not appearing in the scheduler). So it should be easy to get the list from tablegen and exclude the missing instructions with something like:

// Exclude micromips instructions
def : InstRW<[], (instregex ".*_MM$", ".*_MMR[0-9]$")> { let Unsupported = 1; }

Is that okay?

  • Matthias
atrick added a comment.Mar 2 2016, 4:27 PM

Thanks Matthias. If you mark the model complete, won't tablegen give you a list of all unimplemented opcodes without even passing any debug flags to tablgen?

Can you just wrap the whole list of unsupported instruction like this?

let Unsupported = 1 {

def : InstRW...

You can also provide a comma separated list of opcode names or regexes:

def : InstRW<[], (instrs "A", B", ...)>

You don't need a separate InstRW for each.

I think this is a pretty trivial copy-paste step and agree it's better than the dynamic check.

I agree that a static check is generally better than a dynamic check. However we have quite a lot of instructions to mark unsupported due to the way things like MIPS16 and microMIPS are implemented. I'll have a quick look into marking instructions unsupported by feature bit. That should allow us to cover the majority of our unsupported instructions concisely.

I've looked into it and it seems we did have a few holes in our model. In particular, MSA has been overlooked and a few of the rarer instructions are also missing. However, the majority are unsupported on the only processor which currently has a SchedModel.

// Exclude micromips instructions
def : InstRW<[], (instregex ".*_MM$", ".*_MMR[0-9]$")> { let Unsupported = 1; }

Is that okay?

I believe this works by providing an empty list of scheduling information. Won't this disable the dynamic check for these instructions? As far as I can tell, the Unsupported field only serves as documentation.

Another issue is that, some parts of our instruction are difficult to create regexes for. The examples I'm thinking of are:

  • Most of the R6 instructions lack the '_R6' suffix. It was only used where it would otherwise conflict with pre-R6 instructions.
  • DSP/MSA/etc. don't have 'DSP'/'MSA'/etc. in their name. They just use the mnemonic.
  • When we add R6-only processors like the I6400 we'll want to mark the pre-R6 versions of the '*_R6' instructions unsupported.

At the moment, I'm thinking we could add a class that marks all instructions with a specified Predicate as being unsupported. For example:

def : UnsupportedFeatures<"HasMips3">;    // All 64-bit ISA's are unsupported (HasMips3 and any superset are unsupported).
def : UnsupportedFeatures<"HasMips32R6">; // All R6 ISA's are unsupported (HasMips32R6 and any superset are unsupported).
def : UnsupportedFeatures<"HasCnMips">;   // Octeon ASE's are unsupported.
.. etc.

Then CodeGenSchedModels::checkCompleteness() could check for unsupported instructions just after the Inst->hasNoSchedulingInfo check. This suggestion deals with the first two examples, but not the third. I don't have a good solution for that one yet. We could solve it by listing each instruction individually but it would be a shame to have to do that for each and every R6 processor we add in the future.

I think marking the scheduling class as Unsupported will result in invalid scheduling info. It might be good to add a dynamic assert ensuring that all scheduling classes are valid if the model is complete.