[SchedModel] Remove instregex entries that don't match any instructions (WIP)

[RKSimon]

Summary

This patch throws a fatal error if an instregex entry doesn't actually match any instructions. This is part of the work to reduce the compile time impact of increased instregex usage (PR35955), although the x86 models seem to be relatively clean.

I've included hacks for all the cases I encountered, I don't know if I've fixed them correctly though (e.g. AArch64SchedThunderX2T99 has a lot of duplicate patterns for its ALU instructions).

I've also included a disabled error for cases where the instregex only matches one instruction - which IMO should probably be replaced with a instrs entry - this might be worth just a warning or we remove it from the patch completely (but it'd definitely improve compile time).

[andreadb]

The changes to CodeGenSchedule.cpp looks good to me. I will let other people review the AArch64/ARM scheduling model changes.
P.s.: I also like the idea of having a warning (at least in debug mode) for when there is only one matching instruction.

[craig.topper]

utils/TableGen/CodeGenSchedule.cpp

109 ↗

(On Diff #139141)

Do we need two loops here? Why are we building a list of Regexs and the iterating over it?

[RKSimon]

utils/TableGen/CodeGenSchedule.cpp

109 ↗

(On Diff #139141)

I haven't looked into the history of this code, although I think @bkramer refactored some of it not so long ago.

I agree that the loops should probably be merged, I'm happy to cleanup this up as long as it doesn't get in the way of people investigating the issues in the models.

[craig.topper]

I'd like to see the loops merged to get rid of the nested std::pairs. But yeah I don't want to get in the way of fixing all the targets.

The X86 changes look to me. That FMA was one in Zen is interesting. Does the the ZnWriteResFpuPair for FMA have the right information that prevented that from affecting tests?

[RKSimon]

In D44687#1043542, @craig.topper wrote:

I'd like to see the loops merged to get rid of the nested std::pairs. But yeah I don't want to get in the way of fixing all the targets.

The X86 changes look to me. That FMA was one in Zen is interesting. Does the the ZnWriteResFpuPair for FMA have the right information that prevented that from affecting tests?

Yes, the override is probably redundant - see PR35548

[RKSimon]

Rebased

[fhahn]

lib/Target/AArch64/AArch64SchedThunderX2T99.td

426 ↗

(On Diff #139217)

Those changes should be fine (and similar changes below), as they just remove invalid patterns.

1264 ↗

(On Diff #139217)

MOV (AdvSIMD) is an alias for INS. MVN is an alias of NOT. So I think to keep the model unchanged, we should use INS and NOT here.

@joelkevinjones @steleman could you have a quick look to confirm?

Also, the comment just above needs updating.

1516 ↗

(On Diff #139217)

Looks OK, MOVIv should match vector versions of MOVI.

[RKSimon]

@fhahn Would you be willing to commit those scheduler fixes yourself? Ideally I should be able to commit this patch as just the CodeGenSchedule.cpp changes with all the model fixes already in.

[fhahn]

In D44687#1044233, @RKSimon wrote:

@fhahn Would you be willing to commit those scheduler fixes yourself? Ideally I should be able to commit this patch as just the CodeGenSchedule.cpp changes with all the model fixes already in.

Yep, I can commit the AArch64 changes. I'll wait till tomorrow, in case Joel or Stefan have any concerns.

[javed.absar]

Thanks for this. If I am getting this right, just like CompleteModel ensures no instruction has a missing schedule, this patch will ensure no instruction has multiple schedule (instregex) assigned?

lib/Target/ARM/ARMScheduleR52.td

228 ↗

(On Diff #139217)

Not sure why you removed 't2MOV_ga_pcrel_ldr' ? Is it repeated somewhere else ?

287 ↗

(On Diff #139217)

t2MLALBB missing?

utils/TableGen/CodeGenSchedule.cpp

152 ↗

(On Diff #139217)

Just wondering why having 'just one match' would be a problem?

[RKSimon]

In D44687#1044503, @javed.absar wrote:

Thanks for this. If I am getting this right, just like CompleteModel ensures no instruction has a missing schedule, this patch will ensure no instruction has multiple schedule (instregex) assigned?

Not quite (thats another check that I'll be adding soon....) - this patch just errors out if the instregex is not used at all - from the cases I've seen its usually a mixture of copy+paste (not all instructions have all the same variations) and typos in the regex pattern.

[craig.topper]

This patch makes sure each regular expression covers at least one instruction. We already checked that each InstRW line matched at least one instruction. But if there were multiple regular expressions listed, we didn't check it.

Making sure each instruction is only covered by one instregex or instrs match is already being done. And a hole in it was fixed recently with r327808. Though I had to disable the improved check on some models. Look for FullInstRWOverlapCheck = 0 in the SchedMachineModel in the td files to see if I disabled it for any schedulers you care about. I need to collect the effected schedulers and start getting people to fix it.

lib/Target/ARM/ARMScheduleR52.td

228 ↗

(On Diff #139217)

According to ARMGenInstrInfo.inc, there is no t2MOV_ga_pcrel_ldr

287 ↗

(On Diff #139217)

t2MLALBB doesn't exist in ARMGenInstrInfo.inc

utils/TableGen/CodeGenSchedule.cpp

152 ↗

(On Diff #139217)

Having just one match means you should probably be using "(instrs" instead of "(instregex". The regular expression has to get tried on every instruction name that is defined. If there's ever only going to be one match that just wastes tablegen build time.

[RKSimon]

lib/Target/ARM/ARMScheduleR52.td

228 ↗

(On Diff #139217)

Searching through ARMGenInstrInfo.inc I could see MOV_ga_pcrel_ldr but not t2MOV_ga_pcrel_ldr - that was what caused the error.

287 ↗

(On Diff #139217)

Yup - doesn't exist in ARMGenInstrInfo.inc

utils/TableGen/CodeGenSchedule.cpp

152 ↗

(On Diff #139217)

Every regex pattern has to match against every instruction string - its quite costly - we're looking at ways to speed this up (PR35955).

I'm not sure about enabling this - that's (a) why its currently #if'd out and (b) why I might reduce this to a warning if we keep it at all.

[javed.absar]

In D44687#1044554, @craig.topper wrote:

This patch makes sure each regular expression covers at least one instruction. We already checked that each InstRW line matched at least one instruction. But if there were multiple regular expressions listed, we didn't check it.

Making sure each instruction is only covered by one instregex or instrs match is already being done. And a hole in it was fixed recently with r327808. Though I had to disable the improved check on some models. Look for FullInstRWOverlapCheck = 0 in the SchedMachineModel in the td files to see if I disabled it for any schedulers you care about. I need to collect the effected schedulers and start getting people to fix it.

OK, I missed that. Please add R52 to your list. FullInstRWOverlapCheck idea is great. When there is overlap, would it be possible to check if the Defs are identical (less serious case) as opposed to two conflicting sched for same instruction? Sorry, maybe i should ask in a separate review (please point me to it).

[GGanesh]

lib/Target/X86/X86ScheduleZnver1.td

919 ↗

(On Diff #139217)

Looks okay to me.

[fhahn]

In D44687#1044247, @fhahn wrote:

In D44687#1044233, @RKSimon wrote:

@fhahn Would you be willing to commit those scheduler fixes yourself? Ideally I should be able to commit this patch as just the CodeGenSchedule.cpp changes with all the model fixes already in.

Yep, I can commit the AArch64 changes. I'll wait till tomorrow, in case Joel or Stefan have any concerns.

Done, rL328303

[RKSimon]

Rebased after recent scheduler fixes - just the ARM cases still need to be reviewed/committed now.

I've removed the single match code - we can revisit this in the future but for now the number of warnings is just too much for a buildtime performance issues. I'll try to work on at least fixing the x86 cases.

[javed.absar]

LGTM

[GGanesh]

Looks good to me!

llvm/trunk/utils/TableGen/CodeGenSchedule.cpp

for (Init *Arg : make_range(Expr->arg_begin(), Expr->arg_end())) {
         std::string pat = PatStr;
         if (pat[0] != '^') {
           pat.insert(0, "^(");
           pat.insert(pat.end(), ')');
         }
         Regexpr = Regex(pat);
       }
 
+      int NumMatches = 0;
+
       unsigned NumGeneric = Target.getNumFixedInstructions();
       ArrayRef<const CodeGenInstruction *> Generics =
           Target.getInstructionsByEnumValue().slice(0, NumGeneric + 1);
 
       // The generic opcodes are unsorted, handle them manually.
       for (auto *Inst : Generics) {
         StringRef InstName = Inst->TheDef->getName();
         if (InstName.startswith(Prefix) &&
-            (!Regexpr || Regexpr->match(InstName.substr(Prefix.size()))))
+            (!Regexpr || Regexpr->match(InstName.substr(Prefix.size())))) {
           Elts.insert(Inst->TheDef);
+          NumMatches++;
+        }
       }
 
       ArrayRef<const CodeGenInstruction *> Instructions =
           Target.getInstructionsByEnumValue().slice(NumGeneric + 1);
 
       // Target instructions are sorted. Find the range that starts with our
       // prefix.
       struct Comp {
         bool operator()(const CodeGenInstruction *LHS, StringRef RHS) {
           return LHS->TheDef->getName() < RHS;
         }
         bool operator()(StringRef LHS, const CodeGenInstruction *RHS) {
           return LHS < RHS->TheDef->getName() &&
                  !RHS->TheDef->getName().startswith(LHS);
         }
       };
       auto Range = std::equal_range(Instructions.begin(), Instructions.end(),
                                     Prefix, Comp());
 
       // For this range we know that it starts with the prefix. Check if there's
       // a regex that needs to be checked.
       for (auto *Inst : make_range(Range)) {
         StringRef InstName = Inst->TheDef->getName();
-        if (!Regexpr || Regexpr->match(InstName.substr(Prefix.size())))
+        if (!Regexpr || Regexpr->match(InstName.substr(Prefix.size()))) {
           Elts.insert(Inst->TheDef);
+          NumMatches++;
         }
       }
+
+      if (0 == NumMatches)
+        PrintFatalError(Loc, "instregex has no matches: " + Original);
+    }
   }
 };
 
 } // end anonymous namespace
 
 /// CodeGenModels ctor interprets machine model records and populates maps.
 CodeGenSchedModels::CodeGenSchedModels(RecordKeeper &RK,
                                        const CodeGenTarget &TGT):