[TableGen][SchedModels] Fix aliasing of SchedWriteVariant

[evgeny777]

Summary

Atm this doesn't work:

def MyWriteV : SchedWriteVariant<...>;
def : SchedAlias<WriteV, MyWriteV>;

One problem with statements above is that assertion is triggered when inferFromTransitions tries to add empty write sequence which triggers assertion in findOrInsertRW.
Another problem is that transition is always added for given scheduling class, even though that class already has InstRW for a same processor.
Patch attempts to fix both of them

[evgeny777]

Patch also fixes Cortex-A57 model for sxth/uxth/sxtab/uxtab instruction family. Added test case.

[evgeny777]

Reduced test case

[dmgreen]

I don't know this tablegen code very well, but I've read it through and it seems to make sense. I agree that the results look better in line with how the schedule model was written.

llvm/test/TableGen/sched-aliases.td

2

You may need REQUIRES: asserts too, to get dbg output.

48

Is ProcFoo0 needed?

llvm/test/tools/llvm-mca/ARM/A57-sxtb.s

2 ↗

(On Diff #297254)

armv7 -> armv8

Is this file generated with the script? I would expect more tables at the bottom.

Can you pre-commit the file and show the differences here.

llvm/utils/TableGen/CodeGenSchedule.cpp

1678

Could we just not add them to begin with?

[evgeny777]

llvm/test/TableGen/sched-aliases.td

48

Yes. TableGen won't add model which is not bound to any processor

[evgeny777]

llvm/test/tools/llvm-mca/ARM/A57-sxtb.s

2 ↗

(On Diff #297254)

I've reduced test case to make it easier reviewing it.
Pre-commit seems good idea. See 7102793065f2329a2fde78f32a1f2582dd89b0e7

[evgeny777]

Addressed comments

[dmgreen]

LGTM. Cheers

llvm/test/TableGen/sched-aliases.td

48

Sure, but is the subtarget feature needed? I'm pretty sure you can remove this ProcFoo0 and have the features for "foo-0-model" empty.

llvm/test/tools/llvm-mca/ARM/A57-sxtb.s

2 ↗

(On Diff #297254)

Thanks. Like it

[evgeny777]

llvm/test/TableGen/sched-aliases.td

48

Actually it's not. I've removed it in final commit. Thanks.

[RKSimon]

llvm/utils/TableGen/CodeGenSchedule.cpp

1679

@evgeny777 I'm seeing some asserts on msvc builds inside the count() on ARMGenSubtargetInfo.inc generation

[Paul-C-Anagnostopoulos]

This new test, sched-aliases.td, fails when I run the TableGen tests on my machine. You can see the output below. Note that this is the only test file whose RUN line specifies '-o %t' and also pipes the output to FileCheck.

Even if it were to succeed, it takes a long time to run. Is it really necessary to parse the entire AAarch64 .td file? No other test parses a complete target.

I am running on Windows.

---

Command Output (stdout):

$ ":" "RUN: at line 3"
$ "d:\llvm\build\bin\llvm-tblgen.exe" "-gen-instr-info" "C:\LLVM\llvm-project\llvm\test\TableGen\sched-aliases.td" "
-IC:\LLVM\llvm-project\llvm\test\TableGen/../../include" "-IC:\LLVM\llvm-project\llvm\test\TableGen/../../lib/Target
/AArch64" "-o" "D:\LLVM\Build\test\TableGen\Output\sched-aliases.td.tmp" "-debug-only=subtarget-emitter"
note: command had no output on stdout or stderr
error: command failed with exit status: 2147483651
$ "d:\llvm\build\bin\filecheck.exe" "C:\LLVM\llvm-project\llvm\test\TableGen\sched-aliases.td"

--

[evgeny777]

error: command failed with exit status: 2147483651

This means "a breakpoint has been reached" error. Typically it's about hitting llvm_unreachable.
I suggest rynning the test under WinDBG (or Visual Studio debugger if you have it) so we have at least a call trace.

Even if it were to succeed, it takes a long time to run. Is it really necessary to parse the entire AAarch64 .td file? No other test parses a complete target.

Test needs InstRW<>, SchedAlias<> and other scheduler features for which you need to define instructions, register bank, e.t.c. The simplest way to implement
it was including already defined target

[Paul-C-Anagnostopoulos]

I will install winDBG and run the test under it.

I do wish there was a faster way to test what you're testing.

[evgeny777]

I do wish there was a faster way to test what you're testing.

Switching to ARM target (instead of AArch64) makes it twice faster. I can submit a patch for review if you like.

[Paul-C-Anagnostopoulos]

That would be great, thanks.

[dmgreen]

If you are happy with the mca test showing the changes, the other test could be removed if it's causing more trouble than it's worth?

Unoptimized tablegen on windows is known to be very slow. It's up to you but it may be better to create fake instructions or remove the test entirely. Parsing an entire tablegen backend for one test, even if it is a quicker one, might not be something that is great in the long run.

[evgeny777]

If you are happy with the mca test showing the changes, the other test could be removed if it's causing more trouble than it's worth?

Yeah, makes sense
Removal of the test will also fix the issue with crash, BTW

[Paul-C-Anagnostopoulos]

I vote for removing the test if it's not absolutely necessary.

llvm/test/TableGen/sched-aliases.td

+// REQUIRES: aarch64-registered-target
+// RUN: llvm-tblgen -gen-instr-info %s -I%p/../../include -I%p/../../lib/Target/AArch64 -o %t -debug-only=subtarget-emitter 2>&1 | FileCheck %s 
+
+// Check that we've defined scheduling classes for FMOVv2f32_ns and FMOVv2f64 for Model0
+// CHECK: InstRW: New SC [[SC:[0-9]+]]:FMOVv2f32_ns on Model0
+// CHECK: InstRW: New SC [[SC2:[0-9]+]]:FMOVv2f64_ns on Model0
+
+// Generic transition for WriteV should be defined for Model0/ProcFoo0 as well as for
+// all instructions without explicitly defined scheduling classes.
+// CHECK: Adding transition from WriteV({{[0-9]+}}) to Model0WriteV_4cyc({{[0-9]+}}) on processor indices
+// CHECK: Adding transition from WriteV({{[0-9]+}}) to Model0WriteV_2cyc({{[0-9]+}}) on processor indices
+
+// Transition from FMOVv2f64_ns should still be added for Model0/ProcFoo0,
+// even though we've defined custom scheduling class.
+// CHECK: Adding transition from FMOVv2f64_ns([[SC2]]) to Model0WriteV_4cyc({{[0-9]+}}) on processor indices
+// CHECK-NEXT: Adding transition from FMOVv2f64_ns([[SC2]]) to Model0WriteV_2cyc({{[0-9]+}}) on processor indices
+
+// Transition from FMOVv2f32_ns should not be added for Model0/ProcFoo0,
+// because custom sched class for it is defined and it's not variant.
+// CHECK-NOT: Adding transition from FMOVv2f32_ns([[SC]])
+
+include "AArch64.td"
+
+def Model0 : SchedMachineModel {
+  let CompleteModel = 0;
+}
+
+def Model0UnitV    : ProcResource<1> { let BufferSize = 0; }
+
+let SchedModel = Model0 in {
+
+def Model0WriteV_4cyc : SchedWriteRes<[Model0UnitV]> { let Latency = 4; }
+def Model0WriteV_2cyc : SchedWriteRes<[Model0UnitV]> { let Latency = 2; }
+def Model0WriteV_1cyc : SchedWriteRes<[Model0UnitV]> { let Latency = 1; }
+
+def Model0QFormPred : MCSchedPredicate<CheckQForm>;
+def Model0WriteV : SchedWriteVariant<[
+       SchedVar<Model0QFormPred, [Model0WriteV_4cyc]>,
+       SchedVar<NoSchedPred, [Model0WriteV_2cyc]>]>;
+
+def : SchedAlias<WriteV, Model0WriteV>;
+
+def : InstRW<[Model0WriteV_1cyc], (instrs FMOVv2f32_ns)>;
+def : InstRW<[WriteV], (instrs FMOVv2f64_ns)>;
+}
+
+def ProcFoo0 : SubtargetFeature<"foo-0", "ARMProcFamily", "foo-0",
+                                "Test Processor #1", []>;
+
+def : ProcessorModel<"foo-0-model", Model0, [ProcFoo0]>;

llvm/utils/TableGen/CodeGenSchedule.h

struct CodeGenSchedClass {
 
   std::vector<CodeGenSchedTransition> Transitions;
 
   // InstRW records associated with this class. These records may refer to an
   // Instruction no longer mapped to this class by InstrClassMap. These
   // Instructions should be ignored by this class because they have been split
   // off to join another inferred class.
   RecVec InstRWs;
+  // InstRWs processor indices. Filled in inferFromInstRWs
+  DenseSet<unsigned> InstRWProcIndices;
 
   CodeGenSchedClass(unsigned Index, std::string Name, Record *ItinClassDef)
     : Index(Index), Name(std::move(Name)), ItinClassDef(ItinClassDef) {}
 
   bool isKeyEqual(Record *IC, ArrayRef<unsigned> W,
                   ArrayRef<unsigned> R) const {
     return ItinClassDef == IC && makeArrayRef(Writes) == W &&
            makeArrayRef(Reads) == R;

llvm/utils/TableGen/CodeGenSchedule.cpp

for (unsigned I = 0, E = SchedClasses[SCIdx].InstRWs.size(); I != E; ++I) {
     // If this class no longer has any instructions mapped to it, it has become
     // irrelevant.
     if (II == IE)
       continue;
     IdxVec Writes, Reads;
     findRWs(Rec->getValueAsListOfDefs("OperandReadWrites"), Writes, Reads);
     unsigned PIdx = getProcModel(Rec->getValueAsDef("SchedModel")).Index;
     inferFromRW(Writes, Reads, SCIdx, PIdx); // May mutate SchedClasses.
+    SchedClasses[SCIdx].InstRWProcIndices.insert(PIdx);
   }
 }
 
 namespace {
 
 // Helper for substituteVariantOperand.
 struct TransVariant {
   Record *VarOrSeqDef;  // Variant or sequence.
for (SmallVectorImpl<SmallVector<unsigned,4>>::const_iterator
     for (std::vector<PredTransition>::iterator I =
            TransVec.begin() + StartIdx, E = TransVec.end(); I != E; ++I) {
       I->ReadSequences.emplace_back();
     }
     substituteVariantOperand(*RSI, /*IsRead=*/true, StartIdx);
   }
 }
 
+static void addSequences(CodeGenSchedModels &SchedModels,
+                         const SmallVectorImpl<SmallVector<unsigned, 4>> &Seqs,
+                         IdxVec &Result, bool IsRead) {
+  for (const auto &S : Seqs)
+    if (!S.empty())
+      Result.push_back(SchedModels.findOrInsertRW(S, IsRead));
+}
+
+static void dumpTransition(const CodeGenSchedModels &SchedModels,
+                           const CodeGenSchedClass &FromSC,
+                           const CodeGenSchedTransition &SCTrans) {
+  LLVM_DEBUG(dbgs() << "Adding transition from " << FromSC.Name << "("
+                    << FromSC.Index << ") to "
+                    << SchedModels.getSchedClass(SCTrans.ToClassIdx).Name << "("
+                    << SCTrans.ToClassIdx << ")"
+                    << " on processor indices: (";
+             dumpIdxVec(SCTrans.ProcIndices); dbgs() << ")\n");
+}
 // Create a new SchedClass for each variant found by inferFromRW. Pass
 static void inferFromTransitions(ArrayRef<PredTransition> LastTransitions,
                                  unsigned FromClassIdx,
                                  CodeGenSchedModels &SchedModels) {
   // For each PredTransition, create a new CodeGenSchedTransition, which usually
   // requires creating a new SchedClass.
+  const CodeGenSchedClass &FromSC = SchedModels.getSchedClass(FromClassIdx);
   for (ArrayRef<PredTransition>::iterator
          I = LastTransitions.begin(), E = LastTransitions.end(); I != E; ++I) {
-    IdxVec OperWritesVariant;
-    transform(I->WriteSequences, std::back_inserter(OperWritesVariant),
-              [&SchedModels](ArrayRef<unsigned> WS) {
-                return SchedModels.findOrInsertRW(WS, /*IsRead=*/false);
-              });
-    IdxVec OperReadsVariant;
-    transform(I->ReadSequences, std::back_inserter(OperReadsVariant),
-              [&SchedModels](ArrayRef<unsigned> RS) {
-                return SchedModels.findOrInsertRW(RS, /*IsRead=*/true);
-              });
+    IdxVec OperWritesVariant, OperReadsVariant;
+    addSequences(SchedModels, I->WriteSequences, OperWritesVariant, false);
+    addSequences(SchedModels, I->ReadSequences, OperReadsVariant, true);
     CodeGenSchedTransition SCTrans;
+    SCTrans.ProcIndices.assign(I->ProcIndices.begin(), I->ProcIndices.end());
+
+    // Remove all processor indices from this sched transition for which
+    // we also have InstRWs.
+    SCTrans.ProcIndices.erase(
+        llvm::remove_if(SCTrans.ProcIndices,
+                        [&FromSC](unsigned PIdx) {
+                          return FromSC.InstRWProcIndices.count(PIdx);
+                        }),
+        SCTrans.ProcIndices.end());
+    if (SCTrans.ProcIndices.empty())
+      continue;
     SCTrans.ToClassIdx =
         SchedModels.addSchedClass(/*ItinClassDef=*/nullptr, OperWritesVariant,
                                   OperReadsVariant, I->ProcIndices);
-    SCTrans.ProcIndices.assign(I->ProcIndices.begin(), I->ProcIndices.end());
+    dumpTransition(SchedModels, FromSC, SCTrans);
     // The final PredTerm is unique set of predicates guarding the transition.
     RecVec Preds;
     transform(I->PredTerm, std::back_inserter(Preds),
               [](const PredCheck &P) {
                 return P.Predicate;
               });
     Preds.erase(std::unique(Preds.begin(), Preds.end()), Preds.end());
     SCTrans.PredTerm = std::move(Preds);
     SchedModels.getSchedClass(FromClassIdx)
         .Transitions.push_back(std::move(SCTrans));
   }
 }
 
 // Create new SchedClasses for the given ReadWrite list. If any of the
 // ReadWrites refers to a SchedVariant, create a new SchedClass for each variant
 // of the ReadWrite list, following Aliases if necessary.
 void CodeGenSchedModels::inferFromRW(ArrayRef<unsigned> OperWrites,
                                      ArrayRef<unsigned> OperReads,
                                      unsigned FromClassIdx,
                                      ArrayRef<unsigned> ProcIndices) {
   LLVM_DEBUG(dbgs() << "INFER RW proc("; dumpIdxVec(ProcIndices);
              dbgs() << ") ");
-
   // Create a seed transition with an empty PredTerm and the expanded sequences
   // of SchedWrites for the current SchedClass.
   std::vector<PredTransition> LastTransitions;
   LastTransitions.emplace_back();
   LastTransitions.back().ProcIndices.append(ProcIndices.begin(),
                                             ProcIndices.end());
 
   for (unsigned WriteIdx : OperWrites) {