[TableGen][CGS] Print better errors on overlapping InstRW

[evandro]

llvm/utils/TableGen/CodeGenSchedule.cpp

1093

Or rather, continuing from the previous line:

"\"" +
" at " + RWD->getLoc());

1129

Ditto.

[jroelofs]

llvm/utils/TableGen/CodeGenSchedule.cpp

1093

I considered that, but felt that the way the error messages get printed was better when each loc is emitted as a separate diagnostic (so it shows you a bit of relevant source code for each). If there were a proper DiagnosticsEngine I'd prefer to emit this second part as a note:, with the first as the error:, but TableGen diagnostics aren't well set up for that, and adding PrintFatalNote seemed weird.

[evandro]

llvm/utils/TableGen/CodeGenSchedule.cpp

1093

However, I'd rather see the error messages following the same overall pattern: all information pertaining to an error can be found in the same line. It helps when filtering the output of TableGen's.

[nhaehnle]

llvm/utils/TableGen/CodeGenSchedule.cpp

1093

I tend to agree with Jon here. It is in line with the canonical way that compilers tend to report such errors nowadays. I'd rather TableGen move in that direction than do its own thing.

One issue though is that you're now no longer printing the Instrs value as a string. If the RWD record is generated as the result of an mdef, it may be hard to tell which record is really the problem. I'm not familiar enough with the scheduling models to know how problematic this is here, but it'd certainly be a problem for some other TableGen sub-dialects.

[evandro]

llvm/utils/TableGen/CodeGenSchedule.cpp

1093

I'd rather leave the overhaul of how TableGen reports error to a monolithic patch after discussion. I don't believe in moving one error message at a time towards a greater goal.

Yes, it is important to print the instruction value as a string. Thanks for catching that.

[jroelofs]

• Add back the "also matches previous" part of the diagnostics.
• Fix formatting that the linter complains about.

Started a thread here [1] to resolve the difference in preference re: notes vs all-in-one-line errors.

1: http://lists.llvm.org/pipermail/llvm-dev/2020-July/143597.html

[MaskRay]

There probably needs to be a test in llvm/test/TableGen/

[jroelofs]

add test case, clang-format

[evandro]

llvm/lib/TableGen/Error.cpp

53

Please, do not make it fatal.

[jroelofs]

llvm/utils/TableGen/CodeGenSchedule.cpp

1093

Or rather, continuing from the previous line:

"\"" +
" at " + RWD->getLoc());

@evandro what you're asking for here doesn't actually work. Twine doesn't know how to serialize the array of SMLoc's.

[jroelofs]

llvm/lib/TableGen/Error.cpp

53

This is appropriate for places that replace an existing PrintFatalError(); with a PrintError(); PrintFatalNote(); sequence.

If you'd like to take on replacing cases of PrintFatalError() with PrintError(), I'd be happy to review your patch.

[evandro]

llvm/lib/TableGen/Error.cpp

53

Fair enough.

llvm/include/llvm/TableGen/Error.h

 #define LLVM_TABLEGEN_ERROR_H
 
 #include "llvm/Support/SourceMgr.h"
 
 namespace llvm {
 
 void PrintNote(const Twine &Msg);
 void PrintNote(ArrayRef<SMLoc> NoteLoc, const Twine &Msg);
+LLVM_ATTRIBUTE_NORETURN void PrintFatalNote(ArrayRef<SMLoc> ErrorLoc,
+                                            const Twine &Msg);
 
 void PrintWarning(ArrayRef<SMLoc> WarningLoc, const Twine &Msg);
 void PrintWarning(const char *Loc, const Twine &Msg);
 void PrintWarning(const Twine &Msg);
 
 void PrintError(ArrayRef<SMLoc> ErrorLoc, const Twine &Msg);
 void PrintError(const char *Loc, const Twine &Msg);
 void PrintError(const Twine &Msg);

llvm/lib/TableGen/Error.cpp

 }
 
 void PrintNote(const Twine &Msg) { WithColor::note() << Msg << "\n"; }
 
 void PrintNote(ArrayRef<SMLoc> NoteLoc, const Twine &Msg) {
   PrintMessage(NoteLoc, SourceMgr::DK_Note, Msg);
 }
 
+void PrintFatalNote(ArrayRef<SMLoc> NoteLoc, const Twine &Msg) {
+  PrintNote(NoteLoc, Msg);
+  // The following call runs the file cleanup handlers.
+  sys::RunInterruptHandlers();
+  std::exit(1);
+}
+
 void PrintWarning(ArrayRef<SMLoc> WarningLoc, const Twine &Msg) {
   PrintMessage(WarningLoc, SourceMgr::DK_Warning, Msg);
 }
 
 void PrintWarning(const char *Loc, const Twine &Msg) {
   SrcMgr.PrintMessage(SMLoc::getFromPointer(Loc), SourceMgr::DK_Warning, Msg);
 }
 

llvm/test/TableGen/CodeGenSchedule-duplicate-instrw.td

+// RUN: not llvm-tblgen --gen-subtarget -I %p/../../include -I %p/Common %s -o - 2>&1 | FileCheck %s
+
+include "llvm/Target/Target.td"
+
+def FakeTarget : Target { }
+
+def FakeModel : SchedMachineModel { }
+
+def WriteA : SchedWrite;
+def WriteB : SchedWrite;
+
+let SchedModel = NoSchedModel in {
+  def : InstRW<[WriteA], (instrs COPY)>;
+
+  def : InstRW<[WriteB], (instrs COPY)>;
+// CHECK: [[@LINE-1]]:3: error: Overlapping InstRW definition for "COPY" also matches previous "(instrs COPY)".
+// CHECK-NEXT: def : InstRW<[WriteB], (instrs COPY)>;
+
+// CHECK: [[@LINE-6]]:3: note: Previous match was here.
+// CHECK-NEXT: def : InstRW<[WriteA], (instrs COPY)>;
+}
 No newline at end of file

llvm/utils/TableGen/CodeGenSchedule.cpp

for (const Record *R : Decls) {
     StringRef Name = R->getValueAsString("Name");
     const auto It = Declarations.find(Name);
     if (It == Declarations.end()) {
       Declarations[Name] = R;
       continue;
     }
 
     PrintError(R->getLoc(), "STIPredicate " + Name + " multiply declared.");
-    PrintNote(It->second->getLoc(), "Previous declaration was here.");
-    PrintFatalError(R->getLoc(), "Invalid STIPredicateDecl found.");
+    PrintFatalNote(It->second->getLoc(), "Previous declaration was here.");
   }
 
   // Disallow InstructionEquivalenceClasses with an empty instruction list.
   const RecVec Defs =
       Records.getAllDerivedDefinitions("InstructionEquivalenceClass");
   for (const Record *R : Defs) {
     RecVec Opcodes = R->getValueAsListOfDefs("Opcodes");
     if (Opcodes.empty()) {
for (const Record *TIIPred : MCPredicates) {
     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(),
+    PrintFatalNote(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 (OldSCIdx) {
                  "expected a generic SchedClass");
           Record *RWModelDef = InstRWDef->getValueAsDef("SchedModel");
           // Make sure we didn't already have a InstRW containing this
           // instruction on this model.
           for (Record *RWD : RWDefs) {
             if (RWD->getValueAsDef("SchedModel") == RWModelDef &&
                 RWModelDef->getValueAsBit("FullInstRWOverlapCheck")) {
               assert(!InstDefs.empty()); // Checked at function start.
-              PrintFatalError
-                  (InstRWDef->getLoc(),
-                   "Overlapping InstRW definition for \"" +
+              PrintError(
+                  InstRWDef->getLoc(),
                       InstDefs.front()->getName() +
                       "\" also matches previous \"" +
                       RWD->getValue("Instrs")->getValue()->getAsString() +
                       "\".");
+              PrintFatalNote(RWD->getLoc(), "Previous match was here.");
             }
           }
           LLVM_DEBUG(dbgs() << "InstRW: Reuse SC " << OldSCIdx << ":"
                             << SchedClasses[OldSCIdx].Name << " on "
                             << RWModelDef->getName() << "\n");
           SchedClasses[OldSCIdx].InstRWs.push_back(InstRWDef);
           continue;
         }
       }
     }
     unsigned SCIdx = SchedClasses.size();
for (auto &Entry : ClassInstrs) {
     SC.Reads = SchedClasses[OldSCIdx].Reads;
     SC.ProcIndices.push_back(0);
     // If we had an old class, copy it's InstRWs to this new class.
     if (OldSCIdx) {
       Record *RWModelDef = InstRWDef->getValueAsDef("SchedModel");
       for (Record *OldRWDef : SchedClasses[OldSCIdx].InstRWs) {
         if (OldRWDef->getValueAsDef("SchedModel") == RWModelDef) {
           assert(!InstDefs.empty()); // Checked at function start.
-          PrintFatalError
-              (InstRWDef->getLoc(),
-               "Overlapping InstRW definition for \"" +
-               InstDefs.front()->getName() +
-               "\" also matches previous \"" +
+          PrintError(
+              InstRWDef->getLoc(),
+                  InstDefs.front()->getName() + "\" also matches previous \"" +
                   OldRWDef->getValue("Instrs")->getValue()->getAsString() +
                   "\".");
+          PrintFatalNote(OldRWDef->getLoc(), "Previous match was here.");
         }
         assert(OldRWDef != InstRWDef &&
                "SchedClass has duplicate InstRW def");
         SC.InstRWs.push_back(OldRWDef);
       }
     }
     // Map each Instr to this new class.
     for (Record *InstDef : InstDefs)
       InstrClassMap[InstDef] = SCIdx;
     SC.InstRWs.push_back(InstRWDef);
   }
 }