[TableGen] Support warnings on unused classes and multiclasses.

[kosarev]

This still needs some polishing, proper tests and documentation and then maybe separating the AMDGPU changes, but some early feedback might be a help.

[foad]

Nice! This looks very useful as a diagnostic tool. I'm not sure I'd ever want to have it enabled by default.

[kosarev]

Yeah, I'd prefer the checks disabled by default in TableGen, but hope they can always be enabled for AMDGPU so we can catch unused bits as they appear rather than treat them later on occasional checks.

[kosarev]

Separated AMDGPU changes, added tests and documentation.

[foad]

The implementation looks good to me, except perhaps for isInCommonTableGenHeader. It seems like you're inventing a new concept of C-like "system headers", which TableGen did not previously have.

[kosarev]

The implementation looks good to me, except perhaps for isInCommonTableGenHeader. It seems like you're inventing a new concept of C-like "system headers", which TableGen did not previously have.

That was the intention, yes. I think conceptually the common .td files are part of the TableGen backends as much as system headers are part of a C implementation.

However, I guess the purity of the core TableGen code might still be of concern with the isInCommonTableGenHeader() hardcoding the paths, in which case something more explicit would probably work better, like instantiating a special class in such headers maybe?

// In include/llvm/CodeGen/ValueTypes.td.
// The file path in the source position of the definition should now
// be considered a path of a common TableGen header.
def : CommonTableGenHeader<"ValueTypes">;

[kosarev]

Ping.

[nhaehnle]

As shown by the many related revisions, this is clearly a good idea.

I'm wracking my brain as for how the "common" TableGen files can be identified in a cleaner way. I don't like the def way, but something like a pragma common_header perhaps?

[foad]

I'm wracking my brain as for how the "common" TableGen files can be identified in a cleaner way. I don't like the def way, but something like a pragma common_header perhaps?

Maybe you could specify on the command line which files you do want to see warnings in?
llvm-tblgen -warn-in=lib/Target/AMDGPU/ ...

[kosarev]

Maybe you could specify on the command line which files you do want to see warnings in?
llvm-tblgen -warn-in=lib/Target/AMDGPU/ ...

Would be easy to implement, but feels a bit like we ask the user to provide information that should already be known to the system. To be compared with something like gcc --warn-in=foo.c foo.c.

I'm wracking my brain as for how the "common" TableGen files can be identified in a cleaner way. I don't like the def way, but something like a pragma common_header perhaps?

Yeah, a pragma was my first thought as well, so probably should be most natural and expected, though it's yet another concept to be added. Will try that.

[barannikov88]

In D152373#4423873, @foad wrote:

The implementation looks good to me, except perhaps for isInCommonTableGenHeader. It seems like you're inventing a new concept of C-like "system headers", which TableGen did not previously have.

I like it, but maybe instead add the equivalent option (-isystem) and pass it from CMake? Paths may change, and there are also at least clang and mlir that might want to enable this warning.

In D152373#4458773, @nhaehnle wrote:

As shown by the many related revisions, this is clearly a good idea.

I'm wracking my brain as for how the "common" TableGen files can be identified in a cleaner way. I don't like the def way, but something like a pragma common_header perhaps?

"common header" sounds rather vague. Maybe add a way to disable/enable specific warning?

llvm/docs/CommandGuide/tblgen.rst

 .. option:: -time-phases
 
   Time the parser and backend phases and print a report.
 
 .. option:: -version
 
  Show the version number of the program.
 
+.. option:: -warn-on-unused-entities
+
+ Generate warnings on unused classes and multiclasses.
+
 .. option:: -write-if-changed
 
   Write the output file only if it is new or has changed.
 
 
 clang-tblgen Options
 ~~~~~~~~~~~~~~~~~~~~
 

llvm/include/llvm/TableGen/Record.h

private:
   // The DefInit corresponding to this record.
   DefInit *CorrespondingDefInit = nullptr;
 
   // Unique record ID.
   unsigned ID;
 
   bool IsAnonymous;
   bool IsClass;
+  bool IsUsed = false;
 
   void checkName();
 
 public:
   // Constructs a record.
   explicit Record(Init *N, ArrayRef<SMLoc> locs, RecordKeeper &records,
                   bool Anonymous = false, bool Class = false)
       : Name(N), Locs(locs.begin(), locs.end()), TrackedRecords(records),
public:
   // Make the type that this record should have based on its superclasses.
   RecordRecTy *getType();
 
   /// get the corresponding DefInit.
   DefInit *getDefInit();
 
   bool isClass() const { return IsClass; }
 
+  bool isUsed() const { return IsUsed; }
+
+  void setUsed() { IsUsed = true; }
+
   ArrayRef<Init *> getTemplateArgs() const {
     return TemplateArgs;
   }
 
   ArrayRef<RecordVal> getValues() const { return Values; }
 
   ArrayRef<AssertionInfo> getAssertions() const { return Assertions; }
 

llvm/lib/TableGen/Main.cpp

 
 static cl::opt<bool>
 TimePhases("time-phases", cl::desc("Time phases of parser and backend"));
 
 static cl::opt<bool> NoWarnOnUnusedTemplateArgs(
     "no-warn-on-unused-template-args",
     cl::desc("Disable unused template argument warnings."));
 
+static cl::opt<bool>
+    WarnOnUnusedEntities("warn-on-unused-entities",
+                         cl::desc("Enable unused entity warnings."));
+
 static int reportError(const char *ProgName, Twine Msg) {
   errs() << ProgName << ": " << Msg;
   errs().flush();
   return 1;
 }
 
 /// Create a dependency file for `-d` option.
 ///
int llvm::TableGenMain(const char *argv0,
   SrcMgr.setIncludeDirs(IncludeDirs);
 
   TGParser Parser(SrcMgr, MacroNames, Records, NoWarnOnUnusedTemplateArgs);
 
   if (Parser.ParseFile())
     return 1;
   Records.stopTimer();
 
+  if (WarnOnUnusedEntities)
+    Parser.CheckUnusedEntities();
+
   // Write output to memory.
   Records.startBackendTimer("Backend overall");
   std::string OutString;
   raw_string_ostream Out(OutString);
   unsigned status = 0;
   TableGen::Emitter::FnT ActionFn = TableGen::Emitter::Action->getValue();
   if (ActionFn)
     ActionFn(Records, Out);

llvm/lib/TableGen/TGParser.h

struct DefsetRecord {
   SMLoc Loc;
   RecTy *EltTy = nullptr;
   SmallVector<Init *, 16> Elements;
 };
 
 struct MultiClass {
   Record Rec; // Placeholder for template args and Name.
   std::vector<RecordsEntry> Entries;
+  bool IsUsed = false;
 
   void dump() const;
 
   MultiClass(StringRef Name, SMLoc Loc, RecordKeeper &Records)
       : Rec(Name, Loc, Records) {}
 };
 
 class TGVarScope {
TGVarScope *PushScope(MultiClass *Multiclass) {
     return CurScope.get();
   }
   void PopScope(TGVarScope *ExpectedStackTop) {
     assert(ExpectedStackTop == CurScope.get() &&
            "Mismatched pushes and pops of local variable scopes");
     CurScope = CurScope->extractParent();
   }
 
+  void CheckUnusedEntities() const;
+
 private: // Semantic analysis methods.
+  void AddReference(Record *Rec, SMRange Loc);
   bool AddValue(Record *TheRec, SMLoc Loc, const RecordVal &RV);
   /// Set the value of a RecordVal within the given record. If `OverrideDefLoc`
   /// is set, the provided location overrides any existing location of the
   /// RecordVal.
   bool SetValue(Record *TheRec, SMLoc Loc, Init *ValName,
                 ArrayRef<unsigned> BitList, Init *V,
                 bool AllowSelfAssignment = false, bool OverrideDefLoc = true);
   bool AddSubClass(Record *Rec, SubClassReference &SubClass);

llvm/lib/TableGen/TGParser.cpp

Init *TGVarScope::getVar(RecordKeeper &Records, MultiClass* ParsingMultiClass,
   // Then, we try to find the name in parent scope.
   if (Parent)
     return Parent->getVar(Records, ParsingMultiClass, Name, NameLoc,
                           TrackReferenceLocs);
 
   return nullptr;
 }
 
+void TGParser::AddReference(Record *Rec, SMRange Loc) {
+  Rec->setUsed();
+
+  if (TrackReferenceLocs)
+    Rec->appendReferenceLoc(Loc);
+}
+
 bool TGParser::AddValue(Record *CurRec, SMLoc Loc, const RecordVal &RV) {
   if (!CurRec)
     CurRec = &CurMultiClass->Rec;
 
   if (RecordVal *ERV = CurRec->getValue(RV.getNameInit())) {
     // The value already exists in the class, treat this as a set.
     if (ERV->setValue(RV.getValue()))
       return Error(Loc, "New definition of '" + RV.getName() + "' of type '" +
Record *TGParser::ParseClassID() {
   Record *Result = Records.getClass(Lex.getCurStrVal());
   if (!Result) {
     std::string Msg("Couldn't find class '" + Lex.getCurStrVal() + "'");
     if (MultiClasses[Lex.getCurStrVal()].get())
       TokError(Msg + ". Use 'defm' if you meant to use multiclass '" +
                Lex.getCurStrVal() + "'");
     else
       TokError(Msg);
-  } else if (TrackReferenceLocs) {
-    Result->appendReferenceLoc(Lex.getLocRange());
+  } else {
+    AddReference(Result, Lex.getLocRange());
   }
 
   Lex.Lex();
   return Result;
 }
 
 /// ParseMultiClassID - Parse and resolve a reference to a multiclass name.
 /// This returns null on error.
 ///
 ///    MultiClassID ::= ID
 ///
 MultiClass *TGParser::ParseMultiClassID() {
   if (Lex.getCode() != tgtok::Id) {
     TokError("expected name for MultiClassID");
     return nullptr;
   }
 
   MultiClass *Result = MultiClasses[Lex.getCurStrVal()].get();
   if (!Result)
     TokError("Couldn't find multiclass '" + Lex.getCurStrVal() + "'");
 
+  Result->IsUsed = true;
+
   Lex.Lex();
   return Result;
 }
 
 /// ParseSubClassReference - Parse a reference to a subclass or a
 /// multiclass. This returns a SubClassRefTy with a null Record* on error.
 ///
 ///  SubClassRef ::= ClassID
if (Init *I = CurScope->getVar(Records, CurMultiClass, Name, NameLoc,
                                  TrackReferenceLocs))
     return I;
 
   if (Mode == ParseNameMode)
     return Name;
 
   if (Init *I = Records.getGlobal(Name->getValue())) {
     // Add a reference to the global if it's a record.
-    if (TrackReferenceLocs) {
-      if (auto *Def = dyn_cast<DefInit>(I))
-        Def->getDef()->appendReferenceLoc(NameLoc);
-    }
+      AddReference(Def->getDef(), NameLoc);
     return I;
   }
 
   // Allow self-references of concrete defs, but delay the lookup so that we
   // get the correct type.
   if (CurRec && !CurRec->isClass() && !CurMultiClass &&
       CurRec->getNameInit() == Name)
     return UnOpInit::get(UnOpInit::CAST, Name, CurRec->getType());
for (unsigned I = Args.size(), E = TArgs.size(); I < E; ++I) {
       if (!Arg->getValue()->isComplete()) {
         Error(NameLoc.Start, "Value not specified for template argument '" +
                                  TArgs[I]->getAsUnquotedString() + "' (#" +
                                  Twine(I) + ") of parent class '" +
                                  Class->getNameInitAsString() + "'");
       }
     }
 
-    if (TrackReferenceLocs)
-      Class->appendReferenceLoc(NameLoc);
+    AddReference(Class, NameLoc);
     return VarDefInit::get(Class, Args)->Fold();
   }
   case tgtok::l_brace: {           // Value ::= '{' ValueList '}'
     SMLoc BraceLoc = Lex.getLoc();
     Lex.Lex(); // eat the '{'
     SmallVector<Init*, 16> Vals;
 
     if (Lex.getCode() != tgtok::r_brace) {
if (TypedInit *ArgValue = dyn_cast<TypedInit>(Value)) {
                             ArgValue->getAsString());
       }
     }
   }
 
   return false;
 }
 
+static bool isInCommonTableGenHeader(ArrayRef<SMLoc> Loc) {
+  // TODO: Marking common .td files explicitly would be more reliable.
+  auto BufferID = SrcMgr.FindBufferContainingLoc(Loc[0]);
+  assert(BufferID && "Invalid location!");
+  StringRef FileSpec =
+      SrcMgr.getBufferInfo(BufferID).Buffer->getBufferIdentifier();
+  return FileSpec.contains("/include/llvm/CodeGen/") ||
+         FileSpec.contains("/include/llvm/IR/") ||
+         FileSpec.contains("/include/llvm/TableGen/") ||
+         FileSpec.contains("/include/llvm/Target/");
+}
+
+void TGParser::CheckUnusedEntities() const {
+  for (const auto &I : MultiClasses) {
+    ArrayRef<SMLoc> Loc = I.second->Rec.getLoc();
+    if (!I.second->IsUsed && !isInCommonTableGenHeader(Loc))
+      PrintWarning(Loc, "multiclass '" + I.first + "' is unused");
+  }
+
+  for (const auto &I : Records.getClasses()) {
+    ArrayRef<SMLoc> Loc = I.second->getLoc();
+    if (!I.second->isUsed() && !isInCommonTableGenHeader(Loc))
+      PrintWarning(Loc, "class '" + I.first + "' is unused");
+  }
+}
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void RecordsEntry::dump() const {
   if (Loop)
     Loop->dump();
   if (Rec)
     Rec->dump();
 }
 

llvm/test/TableGen/warn-unused-class.td

+// RUN: llvm-tblgen --warn-on-unused-entities %s 2>&1 | \
+// RUN:     FileCheck --implicit-check-not=warning %s
+
+class Used {}
+class Unused {}
+
+def : Used;
+
+// CHECK: warning: class 'Unused' is unused

llvm/test/TableGen/warn-unused-multiclass.td

+// RUN: llvm-tblgen --warn-on-unused-entities %s 2>&1 | \
+// RUN:     FileCheck --implicit-check-not=warning %s
+
+multiclass Used { def foo; }
+multiclass Unused { def foo; }
+
+defm : Used;
+
+// CHECK: warning: multiclass 'Unused' is unused