[MLIR] Add note for file-line numbers in tablegen errors for assembly formats

[stephenneuendorffer]

Summary

Error messages for the custom assembly format are difficult to understand
because there are no line numbers. This happens because the assembly format
is parsed as a standalone line, separate from it's parent file, with no useful
location information. Fixing this properly probably requires quite a bit
of invasive plumbing through the SourceMgr, similar to how included files
are handled

This proposal is a less invasive short term solution. When generating an
error message we generate an additional note which at least properly describes
the operation definition the error occured in, if not the actual line number
of the assemblyFormat definition.

A typical message is like:

error: type of operand #0, named 'operand', is not buildable and a buildable type cannot be inferred

$operand type($result) attr-dict
^

/src/llvm-project/mlir/test/mlir-tblgen/op-format-spec.td:296:1: note: in custom assembly format for this operation
def ZCoverageInvalidC : TestFormat_Op<"variable_invalid_c", [{
^
note: suggest adding a type constraint to the operation or adding a 'type($operand)' directive to the custom assembly format

$operand type($result) attr-dict
^

[stephenneuendorffer]

This would need to be extended to other emitError calls.....

[rriddle]

Thanks!

mlir/tools/mlir-tblgen/OpFormatGen.cpp

1058

Can you populate the location in the constructor so that we can use it for all errors?

1105

nit: Can you move this to after the note below, and add a slightly more descriptive message?

"see the operation defining the assembly format" or something better

[stephenneuendorffer]

Yes, of course, that makes much more sense!

mlir/test/mlir-tblgen/op-format-spec.td

 
 //===----------------------------------------------------------------------===//
 // Directives
 //===----------------------------------------------------------------------===//
 
 //===----------------------------------------------------------------------===//
 // attr-dict
 
-// CHECK: error: 'attr-dict' directive not found in custom assembly format
+// CHECK: error: 'attr-dict' directive not found
 def DirectiveAttrDictInvalidA : TestFormat_Op<"attrdict_invalid_a", [{
 }]>;
 // CHECK: error: 'attr-dict' directive has already been seen
 def DirectiveAttrDictInvalidB : TestFormat_Op<"attrdict_invalid_b", [{
   attr-dict attr-dict
 }]>;
 // CHECK: error: 'attr-dict' directive has already been seen
 def DirectiveAttrDictInvalidC : TestFormat_Op<"attrdict_invalid_c", [{

 // Coverage Checks
 //===----------------------------------------------------------------------===//
 
 // CHECK: error: type of result #0, named 'result', is not buildable and a buildable type cannot be inferred
 // CHECK: note: suggest adding a type constraint to the operation or adding a 'type($result)' directive to the custom assembly format
 def ZCoverageInvalidA : TestFormat_Op<"variable_invalid_a", [{
   attr-dict
 }]>, Arguments<(ins AnyMemRef:$operand)>, Results<(outs AnyMemRef:$result)>;
-// CHECK: error: operand #0, named 'operand', not found in custom assembly format
+// CHECK: error: operand #0, named 'operand', not found
 // CHECK: note: suggest adding a '$operand' directive to the custom assembly format
 def ZCoverageInvalidB : TestFormat_Op<"variable_invalid_b", [{
   type($result) attr-dict
 }]>, Arguments<(ins AnyMemRef:$operand)>, Results<(outs AnyMemRef:$result)>;
 // CHECK: error: type of operand #0, named 'operand', is not buildable and a buildable type cannot be inferred
 // CHECK: note: suggest adding a type constraint to the operation or adding a 'type($operand)' directive to the custom assembly format
 def ZCoverageInvalidC : TestFormat_Op<"variable_invalid_c", [{
   $operand type($result) attr-dict
 }]>, Arguments<(ins AnyMemRef:$operand)>, Results<(outs AnyMemRef:$result)>;
 // CHECK: error: type of operand #0, named 'operand', is not buildable and a buildable type cannot be inferred
 // CHECK: note: suggest adding a type constraint to the operation or adding a 'type($operand)' directive to the custom assembly format
 def ZCoverageInvalidD : TestFormat_Op<"variable_invalid_d", [{
   operands attr-dict
 }]>, Arguments<(ins Variadic<I64>:$operand)>;
 // CHECK: error: type of result #0, named 'result', is not buildable and a buildable type cannot be inferred
 // CHECK: note: suggest adding a type constraint to the operation or adding a 'type($result)' directive to the custom assembly format
 def ZCoverageInvalidE : TestFormat_Op<"variable_invalid_e", [{
   attr-dict
 }]>, Results<(outs Variadic<I64>:$result)>;
-// CHECK: error: successor #0, named 'successor', not found in custom assembly format
+// CHECK: error: successor #0, named 'successor', not found
 // CHECK: note: suggest adding a '$successor' directive to the custom assembly format
 def ZCoverageInvalidF : TestFormat_Op<"variable_invalid_f", [{
    attr-dict
 }]> {
   let successors = (successor AnySuccessor:$successor);
 }
 // CHECK-NOT: error
 def ZCoverageValidA : TestFormat_Op<"variable_valid_a", [{

mlir/tools/mlir-tblgen/OpFormatGen.cpp

private:
   /// A reference to the entire token contents; this is always a pointer into
   /// a memory buffer owned by the source manager.
   StringRef spelling;
 };
 
 /// This class implements a simple lexer for operation assembly format strings.
 class FormatLexer {
 public:
-  FormatLexer(llvm::SourceMgr &mgr);
+  FormatLexer(llvm::SourceMgr &mgr, Operator &op);
 
   /// Lex the next token and return it.
   Token lexToken();
 
   /// Emit an error to the lexer with the given location and message.
   Token emitError(llvm::SMLoc loc, const Twine &msg);
   Token emitError(const char *loc, const Twine &msg);
 
   Token emitErrorAndNote(llvm::SMLoc loc, const Twine &msg, const Twine &note);
 
 private:
   Token formToken(Token::Kind kind, const char *tokStart) {
     return Token(kind, StringRef(tokStart, curPtr - tokStart));
   }
 
   /// Return the next character in the stream.
   int getNextChar();
 
   /// Lex an identifier, literal, or variable.
   Token lexIdentifier(const char *tokStart);
   Token lexLiteral(const char *tokStart);
   Token lexVariable(const char *tokStart);
 
   llvm::SourceMgr &srcMgr;
+  Operator &op;
   StringRef curBuffer;
   const char *curPtr;
 };
 } // end anonymous namespace
 
-FormatLexer::FormatLexer(llvm::SourceMgr &mgr) : srcMgr(mgr) {
+FormatLexer::FormatLexer(llvm::SourceMgr &mgr, Operator &op)
+    : srcMgr(mgr), op(op) {
   curBuffer = srcMgr.getMemoryBuffer(mgr.getMainFileID())->getBuffer();
   curPtr = curBuffer.begin();
 }
 
 Token FormatLexer::emitError(llvm::SMLoc loc, const Twine &msg) {
   srcMgr.PrintMessage(loc, llvm::SourceMgr::DK_Error, msg);
+  llvm::SrcMgr.PrintMessage(op.getLoc()[0], llvm::SourceMgr::DK_Note,
+                            "in custom assembly format for this operation");
   return formToken(Token::error, loc.getPointer());
 }
 Token FormatLexer::emitErrorAndNote(llvm::SMLoc loc, const Twine &msg,
                                     const Twine &note) {
   srcMgr.PrintMessage(loc, llvm::SourceMgr::DK_Error, msg);
+  llvm::SrcMgr.PrintMessage(op.getLoc()[0], llvm::SourceMgr::DK_Note,
+                            "in custom assembly format for this operation");
   srcMgr.PrintMessage(loc, llvm::SourceMgr::DK_Note, note);
   return formToken(Token::error, loc.getPointer());
 }
 Token FormatLexer::emitError(const char *loc, const Twine &msg) {
   return emitError(llvm::SMLoc::getFromPointer(loc), msg);
 }
 
 int FormatLexer::getNextChar() {

 }
 
 namespace {
 /// This class implements a parser for an instance of an operation assembly
 /// format.
 class FormatParser {
 public:
   FormatParser(llvm::SourceMgr &mgr, OperationFormat &format, Operator &op)
-      : lexer(mgr), curToken(lexer.lexToken()), fmt(format), op(op),
+      : lexer(mgr, op), curToken(lexer.lexToken()), fmt(format), op(op),
         seenOperandTypes(op.getNumOperands()),
         seenResultTypes(op.getNumResults()) {}
 
   /// Parse the operation assembly format.
   LogicalResult parse();
 
 private:
   /// This struct represents a type resolution instance. It includes a specific
LogicalResult FormatParser::verifyOperands(
   auto &buildableTypes = fmt.buildableTypes;
   for (unsigned i = 0, e = op.getNumOperands(); i != e; ++i) {
     NamedTypeConstraint &operand = op.getOperand(i);
 
     // Check that the operand itself is in the format.
     if (!hasAllOperands && !seenOperands.count(&operand)) {
       return emitErrorAndNote(loc,
                               "operand #" + Twine(i) + ", named '" +
-                                  operand.name +
-                                  "', not found in custom assembly format",
+                                  operand.name + "', not found",
                               "suggest adding a '$" + operand.name +
                                   "' directive to the custom assembly format");
     }
 
     // Check that the operand type is in the format, or that it can be inferred.
     if (fmt.allOperandTypes || seenOperandTypes.test(i))
       continue;
 
LogicalResult FormatParser::verifySuccessors(llvm::SMLoc loc) {
   if (hasAllSuccessors)
     return success();
 
   for (unsigned i = 0, e = op.getNumSuccessors(); i != e; ++i) {
     const NamedSuccessor &successor = op.getSuccessor(i);
     if (!seenSuccessors.count(&successor)) {
       return emitErrorAndNote(loc,
                               "successor #" + Twine(i) + ", named '" +
-                                  successor.name +
-                                  "', not found in custom assembly format",
+                                  successor.name + "', not found",
                               "suggest adding a '$" + successor.name +
                                   "' directive to the custom assembly format");
     }
   }
   return success();
 }
 
 void FormatParser::handleAllTypesMatchConstraint(