[Tablegen][MCInstPredicate] Removed redundant template argument from class TIIPredicate, and implemented verification rules for TIIPredicates in CodeGenSchedule.cpp.

[andreadb]

Summary

This patch removes redundant template argument TargetName from TIIPredicate.
Tablegen can always infer the target name from the context. So we don't need to force users of TIIPredicate to always specify it.

This allows us to better modularize the tablegen class hierarchy for so-called "function predicates". class FunctionPredicateBase has been added; it is currently used as a building block for TIIPredicates. However, I plan to reuse that class to define other function predicates classes (i.e. not just TIIPredicates).
For example, this can be a first step towards implementing proper support for dependency breaking instructions in tablegen.

This patch also adds a verification step on TIIPredicates in tablegen.
We cannot have multiple TIIPredicates with the same name. Otherwise, this will cause build errors later on, when tablegen'd .inc files are included by cpp files and then compiled.

For example, if I add the following conflicting TIIPredicate at the end of X86SchedPredicates.td

def : TIIPredicate<"isThreeOperandsLEA", MCReturnStatement<TruePred>>;

tablegen (-gen-instr-info) now generates the following error:

Included from /home/andrea/llvm/lib/Target/X86/X86.td:405:
/home/andrea/llvm/lib/Target/X86/X86SchedPredicates.td:58:1: error: TIIPredicate isThreeOperandsLEA is multiply defined.
def : TIIPredicate<"isThreeOperandsLEA", MCReturnStatement<TruePred>>;
^
Included from /home/andrea/llvm/lib/Target/X86/X86.td:405:
/home/andrea/llvm/lib/Target/X86/X86SchedPredicates.td:55:1: note:  Previous definition of isThreeOperandsLEA was here.
def IsThreeOperandsLEAFn :
^
Included from /home/andrea/llvm/lib/Target/X86/X86.td:405:
/home/andrea/llvm/lib/Target/X86/X86SchedPredicates.td:58:1: error: Found conflicting definitions of TIIPredicate.
def : TIIPredicate<"isThreeOperandsLEA", MCReturnStatement<TruePred>>;
^

Please let me know if okay to commit.

Thanks,

• Andrea

[mattd]

I don't see anything wrong with this. I'm all for better modularity with the tablgen "function predicates"; LGTM.

[andreadb]

Thanks for the review Matt.

I noticed that I forgot to mark the new method 'const'. So I have decided to update the code review before committing it.

Changes are:

• s/checkMCPredicates/checkMCInstPredicates
• add "const" to method checkMCInstPredicates
• add "const" to the Record* in the foreach loop

The rest should be identical.
If okay for you, then I would commit this version.

Sorry for the confusion and thanks in advance.

-Andrea

[mattd]

In D50708#1199473, @andreadb wrote:

Thanks for the review Matt.

I noticed that I forgot to mark the new method 'const'. So I have decided to update the code review before committing it.

Even better. LGTM.

include/llvm/Target/TargetInstrPredicate.td

 // `cases` list in input. A default case is automatically generated, and it
 // evaluates to `default`.
 class MCOpcodeSwitchStatement<list<MCOpcodeSwitchCase> cases,
                               MCStatement default> : MCStatement {
   list<MCOpcodeSwitchCase> Cases = cases;
   MCStatement DefaultCase = default;
 }
 
+// Base class for function predicates.
+class FunctionPredicateBase<string name, MCStatement body> {
+  string FunctionName = name;
+  MCStatement Body = body;
+}
+
 // Check that a call to method `Name` in class "XXXGenInstrInfo" (where XXX is
-// the `Target` name) returns true.
+// the name of a target) returns true.
 //
 // TIIPredicate definitions are used to model calls to the target-specific
 // InstrInfo. A TIIPredicate is treated specially by the InstrInfoEmitter
 // tablegen backend, which will use it to automatically generate a definition in
 // the target specific `GenInstrInfo` class.
-class TIIPredicate<string Target, string Name, MCStatement body>
-    : MCInstPredicate {
-  string TargetName = Target;
-  string FunctionName = Name;
-  MCStatement Body = body;
-}
+//
+// There cannot be multiple TIIPredicate definitions with the same name for the
+// same target.
+class TIIPredicate<string Name, MCStatement body>
+    : FunctionPredicateBase<Name, body>, MCInstPredicate;
 
 // A function predicate that takes as input a machine instruction, and returns
 // a boolean value.
 //
 // This predicate is expanded into a function call by the PredicateExpander.
 // In particular, the PredicateExpander would either expand this predicate into
 // a call to `MCInstFn`, or into a call to`MachineInstrFn` depending on whether
 // it is lowering predicates for MCInst or MachineInstr.
 //
 // In this context, `MCInstFn` and `MachineInstrFn` are both function names.
 class CheckFunctionPredicate<string MCInstFn, string MachineInstrFn> : MCInstPredicate {
   string MCInstFnName = MCInstFn;
   string MachineInstrFnName = MachineInstrFn;
 }

lib/Target/X86/X86SchedPredicates.td

 // Used to generate the body of a TII member function.
 def IsThreeOperandsLEABody :
     MCOpcodeSwitchStatement<[LEACases], MCReturnStatement<FalsePred>>;
 
 // This predicate evaluates to true only if the input machine instruction is a
 // 3-operands LEA.  Tablegen automatically generates a new method for it in
 // X86GenInstrInfo.
 def IsThreeOperandsLEAFn :
-    TIIPredicate<"X86", "isThreeOperandsLEA", IsThreeOperandsLEABody>;
+    TIIPredicate<"isThreeOperandsLEA", IsThreeOperandsLEABody>;

utils/TableGen/CodeGenSchedule.h

private:
   void collectProcItins();
 
   void collectProcItinRW();
 
   void collectProcUnsupportedFeatures();
 
   void inferSchedClasses();
 
+  void checkMCInstPredicates() const;
+  
   void checkCompleteness();
 
   void inferFromRW(ArrayRef<unsigned> OperWrites, ArrayRef<unsigned> OperReads,
                    unsigned FromClassIdx, ArrayRef<unsigned> ProcIndices);
   void inferFromItinClass(Record *ItinClassDef, unsigned FromClassIdx);
   void inferFromInstRWs(unsigned SCIdx);
 
   bool hasSuperGroup(RecVec &SubUnits, CodeGenProcModel &PM);

utils/TableGen/CodeGenSchedule.cpp

CodeGenSchedModels::CodeGenSchedModels(RecordKeeper &RK,
   // ProcResourceDefs.
   LLVM_DEBUG(
       dbgs() << "\n+++ RESOURCE DEFINITIONS (collectProcResources) +++\n");
   collectProcResources();
 
   // Collect optional processor description.
   collectOptionalProcessorInfo();
 
+  // Check MCInstPredicate definitions.
+  checkMCInstPredicates();
+
   checkCompleteness();
 }
 
+void CodeGenSchedModels::checkMCInstPredicates() const {
+  RecVec MCPredicates = Records.getAllDerivedDefinitions("TIIPredicate");
+  if (MCPredicates.empty())
+    return;
+
+  // A target cannot have multiple TIIPredicate definitions with a same name.
+  llvm::StringMap<const Record *> TIIPredicates(MCPredicates.size());
+  for (const Record *TIIPred : MCPredicates) {
+    StringRef Name = TIIPred->getValueAsString("FunctionName");
+    StringMap<const Record *>::const_iterator It = TIIPredicates.find(Name);
+    if (It == TIIPredicates.end()) {
+      TIIPredicates[Name] = TIIPred;
+      continue;
+    }
+
+    PrintError(TIIPred->getLoc(),
+               "TIIPredicate " + Name + " is multiply defined.");
+    PrintNote(It->second->getLoc(),
+              " Previous definition of " + Name + " was here.");
+    PrintFatalError(TIIPred->getLoc(),
+                    "Found conflicting definitions of TIIPredicate.");
+  }
+}
+
 void CodeGenSchedModels::collectRetireControlUnits() {
   RecVec Units = Records.getAllDerivedDefinitions("RetireControlUnit");
 
   for (Record *RCU : Units) {
     CodeGenProcModel &PM = getProcModel(RCU->getValueAsDef("SchedModel"));
     if (PM.RetireControlUnit) {
       PrintError(RCU->getLoc(),
                  "Expected a single RetireControlUnit definition");

utils/TableGen/InstrInfoEmitter.cpp

typedef std::map<std::map<unsigned, unsigned>,
                    std::vector<std::string>> OpNameMapTy;
   typedef std::map<std::string, unsigned>::iterator StrUintMapIter;
 
   /// Generate member functions in the target-specific GenInstrInfo class.
   ///
   /// This method is used to custom expand TIIPredicate definitions.
   /// See file llvm/Target/TargetInstPredicates.td for a description of what is
   /// a TIIPredicate and how to use it.
-  void emitTIIHelperMethods(raw_ostream &OS);
+  void emitTIIHelperMethods(raw_ostream &OS, StringRef TargetName);
 
   /// Expand TIIPredicate definitions to functions that accept a const MCInst
   /// reference.
-  void emitMCIIHelperMethods(raw_ostream &OS);
+  void emitMCIIHelperMethods(raw_ostream &OS, StringRef TargetName);
   void emitRecord(const CodeGenInstruction &Inst, unsigned Num,
                   Record *InstrInfo,
                   std::map<std::vector<Record*>, unsigned> &EL,
                   const OperandInfoMapTy &OpInfo,
                   raw_ostream &OS);
   void emitOperandTypesEnum(raw_ostream &OS, const CodeGenTarget &Target);
   void initOperandMapData(
             ArrayRef<const CodeGenInstruction *> NumberedInstructions,
void InstrInfoEmitter::emitOperandTypesEnum(raw_ostream &OS,
 
   OS << "  OPERAND_TYPE_LIST_END" << "\n};\n";
   OS << "} // end namespace OpTypes\n";
   OS << "} // end namespace " << Namespace << "\n";
   OS << "} // end namespace llvm\n";
   OS << "#endif // GET_INSTRINFO_OPERAND_TYPES_ENUM\n\n";
 }
 
-void InstrInfoEmitter::emitMCIIHelperMethods(raw_ostream &OS) {
+void InstrInfoEmitter::emitMCIIHelperMethods(raw_ostream &OS,
+                                             StringRef TargetName) {
   RecVec TIIPredicates = Records.getAllDerivedDefinitions("TIIPredicate");
   if (TIIPredicates.empty())
     return;
 
-  CodeGenTarget &Target = CDP.getTargetInfo();
-  const StringRef TargetName = Target.getName();
-
   OS << "#ifdef GET_GENINSTRINFO_MC_DECL\n";
   OS << "#undef GET_GENINSTRINFO_MC_DECL\n\n";
 
   OS << "namespace llvm {\n";
   OS << "class MCInst;\n\n";
 
   OS << "namespace " << TargetName << "_MC {\n\n";
 
   for (const Record *Rec : TIIPredicates) {
     OS << "bool " << Rec->getValueAsString("FunctionName")
         << "(const MCInst &MI);\n";
   }
 
   OS << "\n} // end " << TargetName << "_MC namespace\n";
   OS << "} // end llvm namespace\n\n";
 
   OS << "#endif // GET_GENINSTRINFO_MC_DECL\n\n";
 
   OS << "#ifdef GET_GENINSTRINFO_MC_HELPERS\n";
   OS << "#undef GET_GENINSTRINFO_MC_HELPERS\n\n";
 
   OS << "namespace llvm {\n";
   OS << "namespace " << TargetName << "_MC {\n\n";
 
-  PredicateExpander PE;
+  PredicateExpander PE(TargetName);
   PE.setExpandForMC(true);
 
   for (const Record *Rec : TIIPredicates) {
     OS << "bool " << Rec->getValueAsString("FunctionName");
     OS << "(const MCInst &MI) {\n";
 
     OS.indent(PE.getIndentLevel() * 2);
     PE.expandStatement(OS, Rec->getValueAsDef("Body"));
     OS << "\n}\n";
   }
 
   OS << "\n} // end " << TargetName << "_MC namespace\n";
   OS << "} // end llvm namespace\n\n";
 
   OS << "#endif // GET_GENISTRINFO_MC_HELPERS\n";
 }
 
-void InstrInfoEmitter::emitTIIHelperMethods(raw_ostream &OS) {
+void InstrInfoEmitter::emitTIIHelperMethods(raw_ostream &OS,
+                                            StringRef TargetName) {
   RecVec TIIPredicates = Records.getAllDerivedDefinitions("TIIPredicate");
   if (TIIPredicates.empty())
     return;
 
-  PredicateExpander PE;
+  PredicateExpander PE(TargetName);
   PE.setExpandForMC(false);
   PE.setIndentLevel(2);
 
   for (const Record *Rec : TIIPredicates) {
     OS << "\n  static bool " << Rec->getValueAsString("FunctionName");
     OS << "(const MachineInstr &MI) {\n";
 
     OS.indent(PE.getIndentLevel() * 2);
void InstrInfoEmitter::run(raw_ostream &OS) {
 
   std::string ClassName = TargetName + "GenInstrInfo";
   OS << "namespace llvm {\n";
   OS << "struct " << ClassName << " : public TargetInstrInfo {\n"
      << "  explicit " << ClassName
      << "(int CFSetupOpcode = -1, int CFDestroyOpcode = -1, int CatchRetOpcode = -1, int ReturnOpcode = -1);\n"
      << "  ~" << ClassName << "() override = default;\n";
 
-  emitTIIHelperMethods(OS);
+  emitTIIHelperMethods(OS, TargetName);
 
   OS << "\n};\n} // end llvm namespace\n";
 
   OS << "#endif // GET_INSTRINFO_HEADER\n\n";
 
   OS << "#ifdef GET_INSTRINFO_CTOR_DTOR\n";
   OS << "#undef GET_INSTRINFO_CTOR_DTOR\n";
 
void InstrInfoEmitter::run(raw_ostream &OS) {
   OS << "} // end llvm namespace\n";
 
   OS << "#endif // GET_INSTRINFO_CTOR_DTOR\n\n";
 
   emitOperandNameMappings(OS, Target, NumberedInstructions);
 
   emitOperandTypesEnum(OS, Target);
 
-  emitMCIIHelperMethods(OS);
+  emitMCIIHelperMethods(OS, TargetName);
 }
 
 void InstrInfoEmitter::emitRecord(const CodeGenInstruction &Inst, unsigned Num,
                                   Record *InstrInfo,
                          std::map<std::vector<Record*>, unsigned> &EmittedLists,
                                   const OperandInfoMapTy &OpInfo,
                                   raw_ostream &OS) {
   int MinOperands = 0;

utils/TableGen/PredicateExpander.h

 
 class raw_ostream;
 
 class PredicateExpander {
   bool EmitCallsByRef;
   bool NegatePredicate;
   bool ExpandForMC;
   unsigned IndentLevel;
+  StringRef TargetName;
 
   PredicateExpander(const PredicateExpander &) = delete;
   PredicateExpander &operator=(const PredicateExpander &) = delete;
 
 public:
-  PredicateExpander()
+  PredicateExpander(StringRef Target)
       : EmitCallsByRef(true), NegatePredicate(false), ExpandForMC(false),
-        IndentLevel(1U) {}
+        IndentLevel(1U), TargetName(Target) {}
   bool isByRef() const { return EmitCallsByRef; }
   bool shouldNegate() const { return NegatePredicate; }
   bool shouldExpandForMC() const { return ExpandForMC; }
   unsigned getIndentLevel() const { return IndentLevel; }
 
   void setByRef(bool Value) { EmitCallsByRef = Value; }
   void flipNegatePredicate() { NegatePredicate = !NegatePredicate; }
   void setNegatePredicate(bool Value) { NegatePredicate = Value; }
public:
   void expandCheckSameRegOperand(raw_ostream &OS, int First, int Second);
   void expandCheckNumOperands(raw_ostream &OS, int NumOps);
   void expandCheckOpcode(raw_ostream &OS, const Record *Inst);
 
   void expandCheckPseudo(raw_ostream &OS, const RecVec &Opcodes);
   void expandCheckOpcode(raw_ostream &OS, const RecVec &Opcodes);
   void expandPredicateSequence(raw_ostream &OS, const RecVec &Sequence,
                                bool IsCheckAll);
-  void expandTIIFunctionCall(raw_ostream &OS, StringRef TargetName,
-                             StringRef MethodName);
+  void expandTIIFunctionCall(raw_ostream &OS, StringRef MethodName);
   void expandCheckIsRegOperand(raw_ostream &OS, int OpIndex);
   void expandCheckIsImmOperand(raw_ostream &OS, int OpIndex);
   void expandCheckInvalidRegOperand(raw_ostream &OS, int OpIndex);
   void expandCheckFunctionPredicate(raw_ostream &OS, StringRef MCInstFn,
                                     StringRef MachineInstrFn);
   void expandCheckNonPortable(raw_ostream &OS, StringRef CodeBlock);
   void expandPredicate(raw_ostream &OS, const Record *Rec);
   void expandReturnStatement(raw_ostream &OS, const Record *Rec);

utils/TableGen/PredicateExpander.cpp

void PredicateExpander::expandPredicateSequence(raw_ostream &OS,
   OS << '\n';
   decreaseIndentLevel();
   OS.indent(getIndentLevel() * 2);
   OS << ')';
   setNegatePredicate(OldValue);
 }
 
 void PredicateExpander::expandTIIFunctionCall(raw_ostream &OS,
-                                              StringRef TargetName,
                                               StringRef MethodName) {
   OS << (shouldNegate() ? "!" : "");
-  if (shouldExpandForMC())
-    OS << TargetName << "_MC::";
-  else
-    OS << TargetName << "Gen"
-       << "InstrInfo::";
+  OS << TargetName << (shouldExpandForMC() ? "_MC::" : "GenInstrInfo::");
   OS << MethodName << (isByRef() ? "(MI)" : "(*MI)");
 }
 
 void PredicateExpander::expandCheckIsRegOperand(raw_ostream &OS, int OpIndex) {
   OS << (shouldNegate() ? "!" : "") << "MI" << (isByRef() ? "." : "->")
      << "getOperand(" << OpIndex << ").isReg() ";
 }
 
if (Rec->isSubClassOf("CheckFunctionPredicate"))
     return expandCheckFunctionPredicate(
         OS, Rec->getValueAsString("MCInstFnName"),
         Rec->getValueAsString("MachineInstrFnName"));
 
   if (Rec->isSubClassOf("CheckNonPortable"))
     return expandCheckNonPortable(OS, Rec->getValueAsString("CodeBlock"));
 
   if (Rec->isSubClassOf("TIIPredicate"))
-    return expandTIIFunctionCall(OS, Rec->getValueAsString("TargetName"),
-                                 Rec->getValueAsString("FunctionName"));
+    return expandTIIFunctionCall(OS, Rec->getValueAsString("FunctionName"));
 
   llvm_unreachable("No known rules to expand this MCInstPredicate");
 }
 
 } // namespace llvm

utils/TableGen/SubtargetEmitter.cpp

void SubtargetEmitter::emitSchedModelHelpersImpl(
   OS << "  switch (SchedClass) {\n";
   for (unsigned VC : VariantClasses) {
     IdxVec ProcIndices;
     const CodeGenSchedClass &SC = SchedModels.getSchedClass(VC);
     collectProcessorIndices(SC, ProcIndices);
 
     OS << "  case " << VC << ": // " << SC.Name << '\n';
 
-    PredicateExpander PE;
+    PredicateExpander PE(Target);
     PE.setByRef(false);
     PE.setExpandForMC(OnlyExpandMCInstPredicates);
     for (unsigned PI : ProcIndices) {
       OS << "    ";
       // Emit a guard on the processor ID.
       if (PI != 0) {
         OS << (OnlyExpandMCInstPredicates
                    ? "if (CPUID == "