[MLIR] Cleanup mlir-tblgen error messages for custom assembly formats.

[stephenneuendorffer]

Summary

The messages are somewhat cryptic, since they are not complete sentences,
include lots of ambiguous words, like 'format' which are hard to parse,
and include names from the users code which may, or may not make sense in
the context of the message. Start to clean this up and provide some
guidance for fixes.

Also, add a test for one of the messages which didn't have a test at all.

[rriddle]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

1376–1377

Remove the punctuation.

1433–1462

Diagnostics should start with lower case letters.

1436

If we are going to have suggestions, they should likely be notes instead of lumped together with the error message.

[stephenneuendorffer]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

1436

This is in Tablegen, which doesn't have the normal reporting infrastructure. Are you suggesting changing that?

[rriddle]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

1436

Notes are a builtin concept, we just need to use DK_Note as the message type when printing the message to the source manager. We could add an optional Twine for a note diagnostic to the emitError function.

https://github.com/llvm/llvm-project/blob/62f3a9650a9f289a07a5f480764fb655178c2334/mlir/tools/mlir-tblgen/OpFormatGen.cpp#L1092

[rriddle]

Thanks for improving these BTW, they look much better. (my fault)

[stephenneuendorffer]

Is this along the lines of what you were thinking? I'm not necessarily wild about having the location described twice (once in the error and once in the note).

Separately, I'm annoyed by the lack of line numbers. I looked at this, but it seems like there is no good way to create a MemoryBuffer such that it has a Loc relative to an outer file. The include mechanism does something similar, but is probably a little misleading.

[rriddle]

Please run clang-format before submitting.

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

 
 //===----------------------------------------------------------------------===//
 // Directives
 //===----------------------------------------------------------------------===//
 
 //===----------------------------------------------------------------------===//
 // attr-dict
 
-// CHECK: error: format missing 'attr-dict' directive
+// CHECK: error: 'attr-dict' directive not found in custom assembly format
 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", [{

 def VariableInvalidK : TestFormat_Op<"variable_invalid_k", [{
   (`foo` $attr^)? `:` attr-dict
 }]>, Arguments<(ins OptionalAttr<I1Attr>:$attr)>;
 
 //===----------------------------------------------------------------------===//
 // Coverage Checks
 //===----------------------------------------------------------------------===//
 
-// CHECK: error: format missing instance of result #0('result') type
+// 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: format missing instance of operand #0('operand')
+// CHECK: error: operand #0, named 'operand', not found in custom assembly format
+// 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: format missing instance of operand #0('operand') type
+// 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: format missing instance of operand #0('operand') type
+// 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: format missing instance of result #0('result') type
+// 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: 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", [{
   $operand type($operand) type($result) attr-dict
 }]>, Arguments<(ins AnyMemRef:$operand)>, Results<(outs AnyMemRef:$result)>;
 def ZCoverageValidB : TestFormat_Op<"variable_valid_b", [{
   $operand type(operands) type(results) attr-dict
 }]>, Arguments<(ins AnyMemRef:$operand)>, Results<(outs AnyMemRef:$result)>;
 def ZCoverageValidC : TestFormat_Op<"variable_valid_c", [{

mlir/tools/mlir-tblgen/OpFormatGen.cpp

public:
 
   /// 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();
 
FormatLexer::FormatLexer(llvm::SourceMgr &mgr) : srcMgr(mgr) {
   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);
   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);
+  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() {
   char curChar = *curPtr++;
   switch (curChar) {
   default:
if (curToken.getKind() != kind)
       return emitError(curToken.getLoc(), msg);
     consumeToken();
     return success();
   }
   LogicalResult emitError(llvm::SMLoc loc, const Twine &msg) {
     lexer.emitError(loc, msg);
     return failure();
   }
+  LogicalResult emitErrorAndNote(llvm::SMLoc loc, const Twine &msg,
+                                 const Twine &note) {
+    lexer.emitErrorAndNote(loc, msg, note);
+    return failure();
+  }
 
   //===--------------------------------------------------------------------===//
   // Fields
   //===--------------------------------------------------------------------===//
 
   FormatLexer lexer;
   Token curToken;
   OperationFormat &fmt;
while (curToken.getKind() != Token::eof) {
     std::unique_ptr<Element> element;
     if (failed(parseElement(element, /*isTopLevel=*/true)))
       return failure();
     fmt.elements.push_back(std::move(element));
   }
 
   // Check that the attribute dictionary is in the format.
   if (!hasAttrDict)
-    return emitError(loc, "format missing 'attr-dict' directive");
+    return emitError(loc, "'attr-dict' directive not found in "
+                          "custom assembly format");
 
   // Check for any type traits that we can use for inferring types.
   llvm::StringMap<TypeResolutionInstance> variableTyResolver;
   for (const OpTrait &trait : op.getTraits()) {
     const llvm::Record &def = trait.getDef();
     if (def.isSubClassOf("AllTypesMatch")) {
       handleAllTypesMatchConstraint(def.getValueAsListOfStrings("values"),
                                     variableTyResolver);
while (it != e) {
 
       // Traverse into optional groups.
       if (auto *optional = dyn_cast<OptionalElement>(element)) {
         auto elements = optional->getElements();
         iteratorStack.emplace_back(elements.begin(), elements.end());
         break;
       }
 
       // We are checking for an attribute element followed by a `:`, so there is
       // no need to check the end.
       if (it == e && iteratorStack.size() == 1)
         break;
 
       // Check for an attribute with a constant type builder, followed by a `:`.
       auto *prevAttr = dyn_cast<AttributeVariable>(element);
       if (!prevAttr || prevAttr->getTypeBuilder())
         continue;
 
       // Check the next iterator within the stack for literal elements.
       for (auto &nextItPair : iteratorStack) {
         ElementsIterT nextIt = nextItPair.first, nextE = nextItPair.second;
         for (; nextIt != nextE; ++nextIt) {
           // Skip any trailing optional groups or attribute dictionaries.
           if (isa<AttrDictDirective>(*nextIt) || isa<OptionalElement>(*nextIt))
             continue;
 
           // We are only interested in `:` literals.
           auto *literal = dyn_cast<LiteralElement>(&*nextIt);
           if (!literal || literal->getLiteral() != ":")
             break;
 
           // TODO: Use the location of the literal element itself.
           return emitError(
               loc, llvm::formatv("format ambiguity caused by `:` literal found "
                                  "after attribute `{0}` which does not have "
                                  "a buildable type",
                                  prevAttr->getVar()->name));
         }
       }
     }
     if (it == e)
       iteratorStack.pop_back();
   }
   return success();
 }
 
 LogicalResult FormatParser::verifyOperands(
     llvm::SMLoc loc,
     llvm::StringMap<TypeResolutionInstance> &variableTyResolver) {
   // Check that all of the operands are within the format, and their types can
   // be inferred.
   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 emitError(loc, "format missing instance of operand #" + Twine(i) +
-                                "('" + operand.name + "')");
+      return emitErrorAndNote(loc,
+                              "operand #" + Twine(i) + ", named '" +
+                                  operand.name +
+                                  "', not found in custom assembly format",
+                              "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;
 
     // Check to see if we can infer this type from another variable.
     auto varResolverIt = variableTyResolver.find(op.getOperand(i).name);
     if (varResolverIt != variableTyResolver.end()) {
       fmt.operandTypes[i].setVariable(varResolverIt->second.type,
                                       varResolverIt->second.transformer);
       continue;
     }
 
     // Similarly to results, allow a custom builder for resolving the type if
     // we aren't using the 'operands' directive.
     Optional<StringRef> builder = operand.constraint.getBuilderCall();
     if (!builder || (hasAllOperands && operand.isVariadic())) {
-      return emitError(loc, "format missing instance of operand #" + Twine(i) +
-                                "('" + operand.name + "') type");
+      return emitErrorAndNote(
+          loc,
+          "type of operand #" + Twine(i) + ", named '" + operand.name +
+              "', is not buildable and a buildable " +
+              "type cannot be inferred",
+          "suggest adding a type constraint "
+          "to the operation or adding a "
+          "'type($" +
+              operand.name + ")' directive to the " + "custom assembly format");
     }
     auto it = buildableTypes.insert({*builder, buildableTypes.size()});
     fmt.operandTypes[i].setBuilderIdx(it.first->second);
   }
   return success();
 }
 
 LogicalResult FormatParser::verifyResults(
if (varResolverIt != variableTyResolver.end()) {
       continue;
     }
 
     // If the result is not variadic, allow for the case where the type has a
     // builder that we can use.
     NamedTypeConstraint &result = op.getResult(i);
     Optional<StringRef> builder = result.constraint.getBuilderCall();
     if (!builder || result.constraint.isVariadic()) {
-      return emitError(loc, "format missing instance of result #" + Twine(i) +
-                                "('" + result.name + "') type");
+      return emitErrorAndNote(
+          loc,
+          "type of result #" + Twine(i) + ", named '" + result.name +
+              "', is not buildable and a buildable " +
+              "type cannot be inferred",
+          "suggest adding a type constraint "
+          "to the operation or adding a "
+          "'type($" +
+              result.name + ")' directive to the " + "custom assembly format");
     }
     // Note in the format that this result uses the custom builder.
     auto it = buildableTypes.insert({*builder, buildableTypes.size()});
     fmt.resultTypes[i].setBuilderIdx(it.first->second);
   }
   return success();
 }
 
 LogicalResult FormatParser::verifySuccessors(llvm::SMLoc loc) {
   // Check that all of the successors are within the format.
   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 emitError(loc, "format missing instance of successor #" +
-                                Twine(i) + "('" + successor.name + "')");
+      return emitErrorAndNote(loc,
+                              "successor #" + Twine(i) + ", named '" +
+                                  successor.name +
+                                  "', not found in custom assembly format",
+                              "suggest adding a '$" + successor.name +
+                                  "' directive to the custom assembly format");
     }
   }
   return success();
 }
 
 void FormatParser::handleAllTypesMatchConstraint(
     ArrayRef<StringRef> values,
     llvm::StringMap<TypeResolutionInstance> &variableTyResolver) {