[mlir][llvm] Use the tablegen error handling (NFC).

[gysit]

Summary

Use PrintError to extend the error message with location information in
LLVMIRConversionGen.cpp. Additionally, promote potentially user facing
error messages from assertions to real errors.

[ftynse]

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

Also, this turns both assertions and user-oriented errors into the same kind. These have different roles: assertions are meant for mlir-tablegen developers while errors are meant for its clients. If an assertion can be triggered by user input, we should check its condition and either fix the code around or promote it to a user-visible error. But this is orthogonal to how the errors are emitted.

[gysit]

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

I thought the PrintFatalError method is the standard way to handle errors in such tablegen code generation files? It is quite nice in the sense that it adds location info to the error message.

The reason why I started this the change is that asserts did not trigger for me (I have CMAKE_BUILD_TYPE:STRING=Debug and LLVM_ENABLE_ASSERTIONS:BOOL=ON)? It may well be that something with my configuration is wrong though.

There is also a PrintError maybe that one would be a better choice?

[rriddle]

In D136057#3861539, @gysit wrote:

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

I thought the PrintFatalError method is the standard way to handle errors in such tablegen code generation files? It is quite nice in the sense that it adds location info to the error message.

The reason why I started this the change is that asserts did not trigger for me (I have CMAKE_BUILD_TYPE:STRING=Debug and LLVM_ENABLE_ASSERTIONS:BOOL=ON)? It may well be that something with my configuration is wrong though.

There is also a PrintError maybe that one would be a better choice?

It'd be nice to stray away from writing PrintFatalError in any new tablegen code that we write. It's really bad for error handling, and we should really prefer cleaner exits.

[ftynse]

In D136057#3861539, @gysit wrote:

It looks like the point of a custom method was to exit more gracefully than abort. Can you comment on why harder failures are desired?

I thought the PrintFatalError method is the standard way to handle errors in such tablegen code generation files? It is quite nice in the sense that it adds location info to the error message.

It is part of the "old" LLVM error handling, we don't want it in new code. Adding the location is pretty much the only benefit it has.

The reason why I started this the change is that asserts did not trigger for me (I have CMAKE_BUILD_TYPE:STRING=Debug and LLVM_ENABLE_ASSERTIONS:BOOL=ON)? It may well be that something with my configuration is wrong though.

This sounds strange. Maybe try running under the debugger. Also check that the same compiler flags are getting used for tools and for libraries, maybe some cross-compilation related weirdness is involved.

There is also a PrintError maybe that one would be a better choice?

It looks more reasonable, at least it does not abort indiscriminately. I'd keep it wrapped in emitError so it plays nicely with the error return flow. Unless @rriddle has a better idea.

[gysit]

This sounds strange. Maybe try running under the debugger. Also check that the same compiler flags are getting used for tools and for libraries, maybe some cross-compilation related weirdness is involved.

The assertion not triggering was my bad! I had optimized tablegen builds enabled...

Let's stick to emitError then maybe extended a little bit with some location info printing. I will also promote some of the assertions to errors since they seem relevant for tablegen users (I suspect many of them will build with optimized tablegen).

[gysit]

Address comments.

mlir/tools/mlir-tblgen/LLVMIRConversionGen.cpp

 #include "mlir/TableGen/GenInfo.h"
 #include "mlir/TableGen/Operator.h"
 
 #include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 
 using namespace llvm;
 using namespace mlir;
 
-static LogicalResult emitError(const Twine &message) {
-  llvm::errs() << message << "\n";
+static LogicalResult emitError(const Record &record, const Twine &message) {
+  PrintError(&record, message);
   return failure();
 }
 
 namespace {
 // Helper structure to return a position of the substring in a string.
 struct StringLoc {
   size_t pos;
   size_t length;

 }
 
 // Emit to `os` the operator-name driven check and the call to LLVM IRBuilder
 // for one definition of an LLVM IR Dialect operation.
 static LogicalResult emitOneBuilder(const Record &record, raw_ostream &os) {
   auto op = tblgen::Operator(record);
 
   if (!record.getValue("llvmBuilder"))
-    return emitError("no 'llvmBuilder' field for op " + op.getOperationName());
+    return emitError(record, "expected 'llvmBuilder' field");
 
   // Return early if there is no builder specified.
   StringRef builderStrRef = record.getValueAsString("llvmBuilder");
   if (builderStrRef.empty())
     return success();
 
   // Progressively create the builder string by replacing $-variables with
   // value lookups.  Keep only the not-yet-traversed part of the builder pattern
if (isOperandName(op, name)) {
       bs << "opInst.getNumResults() == 1";
     } else if (name == "_location") {
       bs << "opInst.getLoc()";
     } else if (name == "_numOperands") {
       bs << "opInst.getNumOperands()";
     } else if (name == "$") {
       bs << '$';
     } else {
-      return emitError(name + " is neither an argument nor a result of " +
-                       op.getOperationName());
+      return emitError(
+          record, "expected keyword, argument, or result, but got " + name);
     }
     // Finally, only keep the untraversed part of the string.
     builderStrRef = builderStrRef.substr(loc.pos + loc.length);
   }
 
   // Output the check and the rewritten builder string.
   os << "if (auto op = dyn_cast<" << op.getQualCppClassName()
      << ">(opInst)) {\n";

 // build for the given LLVM IR instruction. A condition function `conditionFn`
 // emits a check to verify the opcode or intrinsic identifier of the LLVM IR
 // instruction matches the LLVM dialect operation to build.
 static LogicalResult emitOneMLIRBuilder(const Record &record, raw_ostream &os,
                                         ConditionFn conditionFn) {
   auto op = tblgen::Operator(record);
 
   if (!record.getValue("mlirBuilder"))
-    return emitError("no 'mlirBuilder' field for op " + op.getOperationName());
+    return emitError(record, "expected 'mlirBuilder' field");
 
   // Return early if there is no builder specified.
   StringRef builderStrRef = record.getValueAsString("mlirBuilder");
   if (builderStrRef.empty())
     return success();
 
   // Access the argument index array that maps argument indices to LLVM IR
   // operand indices. If the operation defines no custom mapping, set the array
   // to the identity permutation.
   std::vector<int64_t> llvmArgIndices =
       record.getValueAsListOfInts("llvmArgIndices");
   if (llvmArgIndices.empty())
     append_range(llvmArgIndices, seq<int64_t>(0, op.getNumArgs()));
   if (llvmArgIndices.size() != static_cast<size_t>(op.getNumArgs())) {
     return emitError(
-        "'llvmArgIndices' does not match the number of arguments for op " +
-        op.getOperationName());
+        record,
+        "expected 'llvmArgIndices' size to match the number of arguments");
   }
 
   // Progressively create the builder string by replacing $-variables. Keep only
   // the not-yet-traversed part of the builder pattern to avoid re-traversing
   // the string multiple times.
   std::string builder;
   llvm::raw_string_ostream bs(builder);
   while (StringLoc loc = findNextVariable(builderStrRef)) {
     auto name = loc.in(builderStrRef).drop_front();
     // First, insert the non-matched part as is.
     bs << builderStrRef.substr(0, loc.pos);
     // Then, rewrite the name based on its kind.
     FailureOr<int> argIndex = getArgumentIndex(op, name);
     if (succeeded(argIndex)) {
       // Access the LLVM IR operand that maps to the given argument index using
       // the provided argument indices mapping.
       // FIXME: support trailing variadic arguments.
       int64_t operandIdx = llvmArgIndices[*argIndex];
-      assert(operandIdx >= 0 && "expected argument to have a mapping");
+      if (operandIdx < 0) {
+        return emitError(
+            record, "expected non-negative operand index for argument " + name);
+      }
       assert(!isVariadicOperandName(op, name) && "unexpected variadic operand");
       bs << formatv("processValue(llvmOperands[{0}])", operandIdx);
     } else if (isResultName(op, name)) {
-      assert(op.getNumResults() == 1 &&
-             "expected operation to have one result");
+      if (op.getNumResults() != 1)
+        return emitError(record, "expected op to have one result");
       bs << formatv("mapValue(inst)");
     } else if (name == "_int_attr") {
       bs << "matchIntegerAttr";
     } else if (name == "_resultType") {
       bs << "convertType(inst->getType())";
     } else if (name == "_location") {
       bs << "translateLoc(inst->getDebugLoc())";
     } else if (name == "_builder") {
       bs << "odsBuilder";
     } else if (name == "_qualCppClassName") {
       bs << op.getQualCppClassName();
     } else if (name == "$") {
       bs << '$';
     } else {
-      return emitError(name +
-                       " is not a known keyword, argument, or result of " +
-                       op.getOperationName());
+      return emitError(
+          record, "expected keyword, argument, or result, but got " + name);
     }
     // Finally, only keep the untraversed part of the string.
     builderStrRef = builderStrRef.substr(loc.pos + loc.length);
   }
 
   // Output the check and the builder string.
   os << "if (" << conditionFn(record) << ") {\n";
   os << bs.str() << builderStrRef << "\n";