[mlir][tblgen] Improving error messages

[wrengr]

Summary

This differential improves two error conditions, by detecting them earlier and by providing better messages to help users understand what went wrong.

[jpienaar]

mlir/lib/TableGen/Builder.cpp

32

So this was error case where def was a DefInit but the def was null? (it would be good to add test case for this one)

35

No newline at end.

[wrengr]

mlir/lib/TableGen/Builder.cpp

32

I'm not sure offhand, I was just cribbing off similar checks elsewhere. I'll double check that this actually catches the problem I encountered (since it probably doesn't).

The issue I ran into was getting an inscrutable error about a record missing a type field, with the error message pointing to the record's definition instead of the use-site. The goal of this change is to catch the error at the use-site instead, so we can give a more helpful error message. For my particular problem, the root cause was assuming that AttrOrTypeBuilder.dagParams and AttrOrTypeDef.parameters were the same sort of dag; but they're not, and CArg uses the field name type for what AttrOrTypeParameter calls cppType.

[wrengr]

Adding unit test, and correcting the error detection

[wrengr]

Using not mlir-opt on the RUN line (like op-error.td) so that the test gives successful error code. Though it's not clear to me why that's needed here but not for attr-or-type-format-invalid.td ...

[Mogball]

mlir/lib/TableGen/Builder.cpp

27–30

[wrengr]

Removing unnecessary -asmformat-error-is-fatal=false from the test. (That's the difference re why attr-or-type-format-invalid.td doesn't need the not, whereas op-error.td and this test do)

[wrengr]

mlir/lib/TableGen/Builder.cpp

27–30

Why the braces? AFAIK the style guide says to remove them for single-statement blocks.

[wrengr]

Partially addressing Mogball's comment

[Mogball]

mlir/lib/TableGen/Builder.cpp

27–30

It does say "braces should be used when a single-statement body is complex enough that it becomes difficult to see where the block containing the following statement began" which is kind of vague and it doesn't elaborate on all the cases.

The convention that has been used in MLIR though is that braces should be added if the single statement is more than one line (e.g. https://github.com/llvm/llvm-project/blob/df18167ac56d05f2ab55f9d874aee7ab6d5bc9a2/mlir/lib/Pass/Pass.cpp#L259).

(Although, you will find many examples of code style violations in ODS...)

[wrengr]

mlir/lib/TableGen/Builder.cpp

27–30

Most every analogue I see there all lack braces (e.g., in OpToOpPassAdaptor::run on lines 426–428, 429–431, and 440–444). The only analogue I see with braces is at the bottom of OpPassManagerImpl::finalizePassList (lines 259–264) which is rather more complex than just having a string literal wrap.

Not that I feel strongly about it, I'm just trying to match the style guide.

[wrengr]

adding braces

[jpienaar]

mlir/lib/TableGen/Builder.cpp

24–31

I was curious if this cast was source of issues, e.g., do we need to dyn_cast and check if it was a DefInit.

[wrengr]

mlir/lib/TableGen/Builder.cpp

24–31

In principle, yes that could be an error if users try to write 42:$foo or similar. However, after a bit of checking, it turns out that actually causes the Builder ctor to crash when it tries to call Builder::Parameter::getDefaultValue.

I'm thinking the correct approach to resolving that would be to have the Builder::Parameter ctor check that its def is either StringInit or DefInit (and might as well check for the type and defaultValue fields while at it), rather than adding extra code to check it in Builder::Parameter::getDefaultValue. (Of course, once that's done then that would obviate the extra checking in this method too.) But at that point I'm thinking the Builder::Parameter should just store the cppType and defaultValue directly after verifying they exist in the ctor; which then becomes a rather large change that should be done in a separate CL. Thoughts?

[jpienaar]

Thanks!

mlir/lib/TableGen/Builder.cpp

24–31

SGTM

[rriddle]

mlir/lib/TableGen/AttrOrTypeDef.cpp

85

We don't want to enforce this. Some clients replace the default builders with their own implementations, but still want to use the assembly format. Please revert this check.

[wrengr]

mlir/lib/TableGen/AttrOrTypeDef.cpp

85

Then we should have some other mechanism for determining whether builders of the default prototypes exist, so that errors can still be caught at tablegen-time rather than at C++-compile-time, since the latter does not give particularly useful error messages re tracking down the problem. I'll try to write some code to automatically determine that, since adding a hasBuildersOfDefaultPrototype td-field would be too fragile (e.g., if the user defines a builder of the correct prototype via the builders td-field).

FWIW, I ran into this issue when trying to use AttrOrTypeBuilderWithInferredContext in lieu of the default builders (for a Type which is a simple wrapper of another Type, and hence always has the same context).

mlir/lib/TableGen/AttrOrTypeDef.cpp

if (!parameters.empty() && !hasCppFormat && !hasDeclarativeFormat) {
                       "must specify either 'assemblyFormat' or "
                       "'hasCustomAssemblyFormat' when 'mnemonic' is set");
     }
   } else if (hasCppFormat || hasDeclarativeFormat) {
     PrintFatalError(getLoc(),
                     "'assemblyFormat' or 'hasCustomAssemblyFormat' can only be "
                     "used when 'mnemonic' is set");
   }
-  // Assembly format requires accessors to be generated.
+  // Assembly format parser requires default builders to be generated.
+  if (hasDeclarativeFormat && skipDefaultBuilders()) {
+    PrintFatalError(
+        getLoc(),
+        "'assemblyFormat' requires 'skipDefaultBuilders' to be false");
+  }
+  // Assembly format printer requires accessors to be generated.
   if (hasDeclarativeFormat && !genAccessors()) {
     PrintFatalError(getLoc(),
                     "'assemblyFormat' requires 'genAccessors' to be true");
   }
 }
 
 Dialect AttrOrTypeDef::getDialect() const {
   auto *dialect = dyn_cast<llvm::DefInit>(def->getValue("dialect")->getValue());

mlir/lib/TableGen/Builder.cpp

 //===----------------------------------------------------------------------===//
 // Builder::Parameter
 //===----------------------------------------------------------------------===//
 
 /// Return a string containing the C++ type of this parameter.
 StringRef Builder::Parameter::getCppType() const {
   if (const auto *stringInit = dyn_cast<llvm::StringInit>(def))
     return stringInit->getValue();
   const llvm::Record *record = cast<llvm::DefInit>(def)->getDef();
+  // Inlining the first part of `Record::getValueAsString` to give better
+  // error messages.
+  const llvm::RecordVal *type = record->getValue("type");
+  if (!type || !type->getValue()) {
+    llvm::PrintFatalError("Builder DAG arguments must be either strings or "
+                          "defs which inherit from CArg");
+  }
   return record->getValueAsString("type");
 }
 
 /// Return an optional string containing the default value to use for this
 /// parameter.
 Optional<StringRef> Builder::Parameter::getDefaultValue() const {
   if (isa<llvm::StringInit>(def))
     return llvm::None;
   const llvm::Record *record = cast<llvm::DefInit>(def)->getDef();
   Optional<StringRef> value = record->getValueAsOptionalString("defaultValue");
   return value && !value->empty() ? value : llvm::None;
 }

mlir/test/mlir-tblgen/attr-or-type-builder-invalid.td

+// RUN: not mlir-tblgen -gen-typedef-defs -I %S/../../include %s 2>&1 | FileCheck %s
+
+include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "TestDialect";
+  let cppNamespace = "::test";
+}
+
+class InvalidType<string name> : TypeDef<Test_Dialect, name> {
+  let mnemonic = ?;
+}
+
+// This definition should not generate an error due to the use in `InvalidTypeA`
+// CHECK-NOT: Record `TestParameter' does not have a field named `type'!
+def TestParameter : TypeParameter<"int", "int parameter">;
+
+// Test builder uses wrong record class.
+def InvalidTypeA : InvalidType<"InvalidTypeA"> {
+  let parameters = (ins "int":$v0);
+  let builders = [
+    // CHECK: Builder DAG arguments must be either strings or defs which inherit from CArg
+    TypeBuilder<(ins TestParameter:$arg0), [{
+      return $_get($_ctxt, arg0);
+    }]>
+  ];
+}
+