diff --git a/mlir/lib/TableGen/AttrOrTypeDef.cpp b/mlir/lib/TableGen/AttrOrTypeDef.cpp --- a/mlir/lib/TableGen/AttrOrTypeDef.cpp +++ b/mlir/lib/TableGen/AttrOrTypeDef.cpp @@ -29,6 +29,92 @@ // AttrOrTypeDef //===----------------------------------------------------------------------===// +// TODO(wrengr): make this a method instead? +// +/// Check whether this `AttrOrTypeDef` has a builder that matches +/// the default-builder's prototype (regardless of how that builder +/// is implemented), and print warnings or errors as appropriate. +/// +/// If any builder definitely matches (e.g., when `skipDefaultBuilders=0`), +/// then we emit no warnings/errors. If all the builders definitely +/// don't match, then emits a fatal error. Otherwise, emits a non-fatal +/// warning to help users diagnose what went wrong. This third case +/// arises from the fact that we use string-equality to compare the +/// parameters' C++ types, since multiple distinct strings could resolve +/// to the same type. +static void checkForBuilderWithDefaultPrototype(const AttrOrTypeDef &def) { + // If we generate the default builder, that always matches. + if (!def.skipDefaultBuilders()) + return; + // Cache the results of `AttrOrTypeParameter::getCppType()` to avoid + // repeating work. + const unsigned numDefParams = def.getParameters().size(); + SmallVector defParams; + defParams.reserve(numDefParams); + for (const AttrOrTypeParameter ¶m : def.getParameters()) + defParams.emplace_back(param.getCppType()); + // Check each custom builder. + bool isUnsure = false; + for (const AttrOrTypeBuilder &builder : def.getBuilders()) { + // The default builder has a "::mlir::MLIRContext *" parameter; + // so if this builder doesn't, then it definitely can't match. + // (cf., the analogous logic in `getCustomBuilderParams`) + if (builder.hasInferredContextParameter()) + continue; + const auto builderParams = builder.getParameters(); + const unsigned numBuilderParams = builderParams.size(); + // If this builder has too few parameters, then it definitely can't match. + if (numBuilderParams < numDefParams) + continue; + // If this builder has too many non-default parameters, then it + // definitely can't match. + unsigned i = numDefParams; + for (; i < numBuilderParams; ++i) + if (!builderParams[i].getDefaultValue()) + break; + if (i != numBuilderParams) + continue; + // Now we know we have the right number of parameters; so check each + // builder parameter against its corresponding def parameter. + i = 0; + for (; i < numDefParams; ++i) { + if (builderParams[i].getCppType() != defParams[i]) { + isUnsure = true; // This builder could still match, so take note. + break; + } + } + // If we didn't break out of that loop, then all the required + // parameters are definitely equal; so this builder definitely matches, + // and we're done. + if (i == numDefParams) + return; + } + // After checking all the builders (and finding no definite matches), + // if there's an uncertain match, then only warn rather than erroring. + if (isUnsure) { + PrintWarning(def.getLoc(), + "using 'assemblyFormat' with 'skipDefaultBuilders=1' may " + "result in C++ compilation errors"); + } else { + // If all builders were definite mismatches, then there's no hope. + PrintFatalError(def.getLoc(), + "'assemblyFormat' requires a default-prototype builder"); + /* FIXME(wrengr): to respect the `-asmformat-error-is-fatal=false` + // commandline option we'd need to replace the above `PrintFatalError` + // with something like the following: + #include "mlir/tools/mlir-tblgen/FormatGen.h" // Alas, we can't. + #include "llvm/Support/Signals.h" + PrintError(def.getLoc(), + "'assemblyFormat' requires a default-prototype builder"); + if (mlir::tblgen::formatErrorIsFatal) { + // Invoke the interrupt handlers to run the file cleanup handlers. + llvm::sys::RunInterruptHandlers(); + std::exit(1); + } + */ + } +} + AttrOrTypeDef::AttrOrTypeDef(const llvm::Record *def) : def(def) { // Populate the builders. auto *builderList = @@ -83,12 +169,10 @@ } // Assembly format parser requires builders with the same prototype // as the default-builders. - // TODO: attempt to detect when a custom builder matches the prototype. - if (hasDeclarativeFormat && skipDefaultBuilders()) { - PrintWarning(getLoc(), - "using 'assemblyFormat' with 'skipDefaultBuilders=1' may " - "result in C++ compilation errors"); - } + // FIXME(wrengr): avoid repeated calls to + // `checkForBuilderWithDefaultPrototype` for the same record. + if (hasDeclarativeFormat) + checkForBuilderWithDefaultPrototype(*this); // Assembly format printer requires accessors to be generated. if (hasDeclarativeFormat && !genAccessors()) { PrintFatalError(getLoc(), diff --git a/mlir/test/mlir-tblgen/attr-or-type-format-invalid.td b/mlir/test/mlir-tblgen/attr-or-type-format-invalid.td --- a/mlir/test/mlir-tblgen/attr-or-type-format-invalid.td +++ b/mlir/test/mlir-tblgen/attr-or-type-format-invalid.td @@ -132,3 +132,59 @@ // CHECK: `struct` can only be used at the top-level context let assemblyFormat = "custom(struct(params))"; } + +/* // FIXME(wrengr): +// COM: // CHECK: 'assemblyFormat' requires a default-prototype builder +// COM: // CHECK-NEXT: def InvalidTypeS +def InvalidTypeS : InvalidType<"InvalidTypeS", "invalid_s"> { + let parameters = (ins "int":$a); + let assemblyFormat = "struct(params)"; + // No builders at all. + let skipDefaultBuilders = 1; +} + +// COM: // CHECK: 'assemblyFormat' requires a default-prototype builder +// COM: // CHECK-NEXT: def InvalidTypeT +def InvalidTypeT : InvalidType<"InvalidTypeT", "invalid_t"> { + let parameters = (ins "int":$a); + let assemblyFormat = "struct(params)"; + // Custom builder lacks the context argument. + let skipDefaultBuilders = 1; + let builders = [TypeBuilderWithInferredContext<(ins "int":$a)>]; +} + +// TODO(wrengr): checks for too many/few parameters, and default-valued + +// TODO(wrengr): how to make this check-not match at the right place? +// COM: // CHECK-NOT: 'assemblyFormat' requires a default-prototype builder +def ValidTypeS : TypeDef { + let mnemonic = "valid_s"; + let parameters = (ins "int":$a); + let assemblyFormat = "struct(params)"; + // Default matches. + let skipDefaultBuilders = 0; +} + +// TODO(wrengr): how to make this check-not match at the right place? +// COM: // CHECK-NOT: 'assemblyFormat' requires a default-prototype builder +def ValidTypeT : TypeDef { + let mnemonic = "valid_t"; + let parameters = (ins "int":$a); + let assemblyFormat = "struct(params)"; + // Custom matches exactly. + let skipDefaultBuilders = 1; + let builders = [TypeBuilder<(ins "int":$a)>]; +} + +// COM: // CHECK-NOT: 'assemblyFormat' requires a default-prototype builder +// COM: // CHECK: using 'assemblyFormat' with 'skipDefaultBuilders=1' may result in C++ compilation errors +// COM: // CHECK-NEXT: def ValidTypeU +def ValidTypeU : TypeDef { + let mnemonic = "valid_u"; + let parameters = (ins "int":$a); + let assemblyFormat = "struct(params)"; + // Custom matches, but not exactly. + let skipDefaultBuilders = 1; + let builders = [TypeBuilder<(ins "signed int":$a)>]; +} +// */