[SchedModel] Complete models shouldn't match against itineraries when they don't use them (PR35639) (WIP)

[RKSimon]

Summary

For schedule models that don't use itineraries, checkCompleteness still checks that an instruction has a matching itinerary instead of skipping and going straight to matching the InstRWs. That doesn't seem to match what happens in TargetSchedule.cpp

This patch causes problems for a number of models flagged as complete that I've included for reference/discussion.

[uweigand]

Ah, so this is a problem when some processor models use itineraries and others don't? In that case, the change looks good to me ...

[nemanjai]

I was under the impression that the purpose of ItinRW is a way to map processor resources to itineraries that were already defined. This in turn prevents the need to update existing instructions (since they have been defined with a list of itineraries).

Judging by this patch and the need to set the PPC P9 scheduling model to incomplete seems to suggest that this understanding is incorrect.

[atrick]

I can confirm that the purpose of ItinRW is to avoid the need to define InstRW for a subtarget when the target already had itinerary classes. (Things would be *a lot* simpler if all the subtargets used the same style of machine description.)

CompleteModel can mean whatever is currently most useful to devs, but we don't want to lose the scheduling-time checks just because a subtarget doesn't define InstRW for everything. Maybe it would work for those subtargets should have InstRW { let Unsupported = 1 } for opcodes that aren't covered by itinerary classes?

[nemanjai]

In D43235#1008022, @atrick wrote:

I can confirm that the purpose of ItinRW is to avoid the need to define InstRW for a subtarget when the target already had itinerary classes. (Things would be *a lot* simpler if all the subtargets used the same style of machine description.)

CompleteModel can mean whatever is currently most useful to devs, but we don't want to lose the scheduling-time checks just because a subtarget doesn't define InstRW for everything. Maybe it would work for those subtargets should have InstRW { let Unsupported = 1 } for opcodes that aren't covered by itinerary classes?

I am not sure I completely follow what you're suggesting. I'll use Power9 scheduling model in the PPC back end as an example.
We've implemented the model to try to be as accurate as we can. A vast majority of instructions that were defined with the same itinerary use the same functional units, so we just used ItinRW to map instructions with a specific itinerary to specific functional units. For instructions that don't have an itinerary, we've provided an InstRW or set the "no scheduling information" bit. Also, for instructions that have an itinerary, but use different functional units from most other instructions with that itinerary, we've overridden the resources that come from ItinRW with InstRW. This has allowed us to mark the model complete.

Since all of our instructions either had an itinerary specified or have been covered in InstRW (and all our itineraries covered by ItinRW), we assumed that all our instructions have all the processor resources correctly specified. However, judging by this patch and the fact that we don't satisfy the condition on line 1637 (!SC.Writes.empty()), it appears that we actually misunderstood what ItinRW actually does. In fact, it appears to me that a vast majority of our instructions (not covered by InstRW) don't actually provide any information to the scheduler about which functional resources in the CPU they consume. And if so, I am of course very much in favour of this patch as it highlights a problem we didn't know we had.

Is this an accurate understanding? If so, I'm not sure what the actual semantics of ItinRW are. Also, if we have to write InstRW's for all our instructions in our case, we don't mind doing that. I just want to make sure that's what we should be doing before spending time on it.

[RKSimon]

Removed Cortex-A57 model diff now that rL326304 has landed

[stefanp]

With the patch rL327021 we should now be able to mark Power 9 as complete.

[RKSimon]

Removed Power9 model diff now that rL327021 has landed

[RKSimon]

Ping - is anyone wanting to fix the SI or MIPs scheduler models to be complete again before I commit this? @sdardis @arsenm

[sdardis]

@RKSimon , I'm ok with this going in, I'm in process of updating our scheduler models to account for this but it may take me a few days.

[RKSimon]

ping?

[RKSimon]

ping^2

[andreadb]

In D43235#1056571, @RKSimon wrote:

ping^2

Looks good to me.

Thanks!

lib/Target/AMDGPU/SISchedule.td

 // Half rate 64-bit instructions.
 def Write64Bit : SchedWrite;
 
 // FIXME: Should there be a class for instructions which are VALU
 // instructions and have VALU rates, but write to the SALU (i.e. VOPC
 // instructions)
 
 class SISchedMachineModel : SchedMachineModel {
-  let CompleteModel = 1;
+  let CompleteModel = 0;
   // MicroOpBufferSize = 1 means that instructions will always be added
   // the ready queue when they become available.  This exposes them
   // to the register pressure analysis.
   let MicroOpBufferSize = 1;
   let IssueWidth = 1;
   let PostRAScheduler = 1;
 
   // FIXME:Approximate 2 * branch cost.  Try to hack around bad

lib/Target/Mips/MipsScheduleGeneric.td

def MipsGenericModel : SchedMachineModel {
 
   // These figures assume an L1 hit.
   int LoadLatency = 2;
   int MispredictPenalty = 4;
 
   int HighLatency = 37;
   list<Predicate> UnsupportedFeatures = [];
 
-  let CompleteModel = 1;
+  let CompleteModel = 0;
   let PostRAScheduler = 1;
 }
 
 let SchedModel = MipsGenericModel in {
 
 // ALU Pipeline
 // ============
 

lib/Target/Mips/MipsScheduleP5600.td

 //==- MipsScheduleP5600.td - P5600 Scheduling Definitions --*- tablegen -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 def MipsP5600Model : SchedMachineModel {
   int IssueWidth = 2; // 2x dispatched per cycle
   int MicroOpBufferSize = 48; // min(48, 48, 64)
   int LoadLatency = 4;
   int MispredictPenalty = 8; // TODO: Estimated
 
-  let CompleteModel = 1;
+  let CompleteModel = 0;
 
   list<Predicate> UnsupportedFeatures = [HasMips32r6, HasMips64r6,
                                          HasMips64, HasMips64r2, HasCnMips,
                                          InMicroMips, InMips16Mode,
                                          HasDSP, HasDSPR2, HasMT];
 
 }
 

utils/TableGen/CodeGenSchedule.cpp

void CodeGenSchedModels::collectProcResources() {
   ProcResourceDefs.clear();
   ProcResGroups.clear();
 }
 
 void CodeGenSchedModels::checkCompleteness() {
   bool Complete = true;
   bool HadCompleteModel = false;
   for (const CodeGenProcModel &ProcModel : procModels()) {
+    const bool HasItineraries = ProcModel.hasItineraries();
     if (!ProcModel.ModelDef->getValueAsBit("CompleteModel"))
       continue;
     for (const CodeGenInstruction *Inst : Target.getInstructionsByEnumValue()) {
       if (Inst->hasNoSchedulingInfo)
         continue;
       if (ProcModel.isUnsupported(*Inst))
         continue;
       unsigned SCIdx = getSchedClassIdx(*Inst);
       if (!SCIdx) {
         if (Inst->TheDef->isValueUnset("SchedRW") && !HadCompleteModel) {
           PrintError("No schedule information for instruction '"
                      + Inst->TheDef->getName() + "'");
           Complete = false;
         }
         continue;
       }
 
       const CodeGenSchedClass &SC = getSchedClass(SCIdx);
       if (!SC.Writes.empty())
         continue;
-      if (SC.ItinClassDef != nullptr &&
+      if (HasItineraries && SC.ItinClassDef != nullptr &&
           SC.ItinClassDef->getName() != "NoItinerary")
         continue;
 
       const RecVec &InstRWs = SC.InstRWs;
       auto I = find_if(InstRWs, [&ProcModel](const Record *R) {
         return R->getValueAsDef("SchedModel") == ProcModel.ModelDef;
       });
       if (I == InstRWs.end()) {