[PATCH] D77488: [MLIR] Proposal for file-line numbers in tablegen errors.

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer created this revision.
Herald added subscribers: llvm-commits, grosul1, Joonsoo, liufengdb, lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, burmako, jpienaar, rriddle, mehdi_amini.
Herald added a project: LLVM.
stephenneuendorffer added a reviewer: rriddle.
stephenneuendorffer added a comment.

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


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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77488

Files:
  mlir/tools/mlir-tblgen/OpFormatGen.cpp


Index: mlir/tools/mlir-tblgen/OpFormatGen.cpp
===================================================================
--- mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1057,7 +1057,8 @@
   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);
+  Token emitErrorAndNote(Operator &op, llvm::SMLoc loc,
+                         const Twine &msg, const Twine &note);
 
 private:
   Token formToken(Token::Kind kind, const char *tokStart) {
@@ -1087,9 +1088,13 @@
   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) {
+Token FormatLexer::emitErrorAndNote(Operator &op,
+                                    llvm::SMLoc loc,
+                                    const Twine &msg,
+                                    const Twine &note) {
+  llvm::SrcMgr.PrintMessage(op.getLoc()[0],
+                            llvm::SourceMgr::DK_Note,
+                            "here");
   srcMgr.PrintMessage(loc, llvm::SourceMgr::DK_Error, msg);
   srcMgr.PrintMessage(loc, llvm::SourceMgr::DK_Note, note);
   return formToken(Token::error, loc.getPointer());
@@ -1311,10 +1316,11 @@
     lexer.emitError(loc, msg);
     return failure();
   }
-  LogicalResult emitErrorAndNote(llvm::SMLoc loc,
+  LogicalResult emitErrorAndNote(Operator &op,
+                                 llvm::SMLoc loc,
                                  const Twine &msg,
                                  const Twine &note) {
-    lexer.emitErrorAndNote(loc, msg, note);
+    lexer.emitErrorAndNote(op, loc, msg, note);
     return failure();
   }
 
@@ -1394,7 +1400,7 @@
       NamedTypeConstraint &result = op.getResult(i);
       Optional<StringRef> builder = result.constraint.getBuilderCall();
       if (!builder || result.constraint.isVariadic()) {
-        return emitErrorAndNote(loc,
+        return emitErrorAndNote(op, loc,
                                 "type of result #" + Twine(i) +
                                 ", named '" + result.name +
                                 "', is not buildable and a buildable " +
@@ -1418,7 +1424,7 @@
 
     // Check that the operand itself is in the format.
     if (!hasAllOperands && !seenOperands.count(&operand)) {
-      return emitErrorAndNote(loc, "operand #" + Twine(i) +
+      return emitErrorAndNote(op, loc, "operand #" + Twine(i) +
                               ", named '" + operand.name +
                               "', not found in custom assembly format",
                               "suggest adding a '$" + operand.name +
@@ -1441,7 +1447,7 @@
     // we aren't using the 'operands' directive.
     Optional<StringRef> builder = operand.constraint.getBuilderCall();
     if (!builder || (hasAllOperands && operand.isVariadic())) {
-      return emitErrorAndNote(loc,
+      return emitErrorAndNote(op, loc,
                               "type of operand #" + Twine(i) +
                               ", named '" + operand.name +
                               "', is not buildable and a buildable " +
@@ -1461,7 +1467,7 @@
     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) +
+        return emitErrorAndNote(op, loc, "successor #" + Twine(i) +
                                 ", named '" + successor.name +
                                 "', not found in custom assembly format",
                                 "suggest adding a '$" + successor.name +


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77488.255117.patch
Type: text/x-patch
Size: 3907 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200405/6937f4cb/attachment.bin>