[mlir][tblgen] Improve detecting default-prototype builder for AttrOrTypeDef

[wrengr]

Summary

First attempt at resolving https://llvm.org/PR56415 , though it's still a bit WIP at the moment.

[wrengr]

This differential is still a bit WIP regarding style (i.e., whether to make the new functions methods or not, names of things, etc). Also I still need to add some unit tests to ensure this works as intended.

@rriddle let me know if this general approach looks reasonable to you, and if so then offer any thoughts you have re the style portion of things.

[jpienaar]

mlir/lib/TableGen/AttrOrTypeDef.cpp

35

Yeah folks were mentioning desire to move away from strings in that context and more similar to how this is handled in arguments.

48

Could we inline the call here? That seems like it would be easier to follow and avoid the need for introducing K3

63

Are there any warnings flagged here?

[rriddle]

mlir/lib/TableGen/AttrOrTypeDef.cpp

35

Yeah, I think for now it would be nice if we just check endsWith instead of ==. That might cover most uses, where the generic builders have namespaces (e.g. "::mlir::MLIRContext *":$ctx) and the user written builders don't (e.g. "MLIRContext *":$ctx).

[Mogball]

mlir/lib/TableGen/AttrOrTypeDef.cpp

44

Checking for the string equality of the C++ types can be fragile, because it doesn't account for namespace differences, e.g. ::mlir::Type vs Type, and cannot account for implicit C++ conversions, e.g. SmallVectorImpl<T> & and ArrayRef<T>.

[wrengr]

Lots of changes/improvements.

Okay, so this is starting to turn into a rabbit hole, even without getting into any more sophisticated string comparisons. Here are the problems I'm running into at the moment:

1. Errors emitted by the AttrOrTypeDef ctor occur before the errors emitted by mlir::tblgen::generateAttrOrTypeFormat, therefore the test/mlir-tblgen/attr-or-type-format-invalid.td file can't include tests for errors emitted by both sources (without implicitly ordering the seemingly-independent tests so that their errors occur in FileCheck order).

2. Fatal errors emitted by AttrOrTypeDef don't respect the -asmformat-error-is-fatal=false commandline option, so they cause mlir-tblgen to exit before even calling generateAttrOrTypeFormat. The mlir/lib/TableGen/AttrOrTypeDef.cpp file isn't allowed to include the mlir/tools/mlir-tblgen/FormatGen.h header, so it can't access that commandline option directly; so if we want these errors to respect that commandline option, then the only solution I can think of is for the AttrOrTypeDef ctor to take an extra bool argument, and then have the callers thread things through. However, such explicit threading of state is a code smell and will quickly become gross. Alas, without somehow respecting the commandline option, each test record that expects a fatal error would have to be given their own separate files.

3. Relatedly, since the "unsure" case could still have too many false-positives, ideally we'd have some other commandline option for indicating whether the user desires the warnings or not. Which runs into the same issues re threading state around.

4. The biggest issue, however, is that the AttrOrTypeDef ctor is called repeatedly for the same Record. This means that any expensive checks will be run repeatedly, and any non-fatal messages will also be reported repeatedly. I think this one has a feasible solution, though I haven't tried it out yet. Namely: the AttrOrTypeDef ctor seems to only be called by mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:collectAllDefs, and that function seems to only be called by mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp:DefGenerator, which only calls it with the DefGenerator::defRecords field, which itself doesn't seem to be used anywhere else. If that chain of references is indeed the only one, then we could have DefGenerator store the vector of AttrOrTypeDef objects in lieu of the vector of Record*, and adjust the type of collectAllDefs accordingly. This way, the AttrOrTypeDef ctor is only called once on any given record, and the results are cached in the DefGenerator for it to reuse for both decls and defs.

[wrengr]

mlir/lib/TableGen/AttrOrTypeDef.cpp

44

Oh I'm all too aware of the many fragilities of using string comparisons to try emulating type comparisons :) E.g., there's also things like whitespace differences. I have some preliminary work to deal with whitespace and K3 matching of namespaces, but don't want to spend too much time working on that until after determining if we even want to stick with this approach at all (as opposed to, say, adding a new bit hasDefaultPrototypeCustomBuilder field for users to explicitly say when they have some custom builder they expect to match).

Or were you suggesting I explicitly mention namespaces and implicit conversions in the documentation?

48

I was hoping that separating them would help make the logic easier to follow, since the for-loop + switch is just the K3 version of llvm::any_of. (And factoring out the builderHasParameters predicate follows the general style guide of factoring out predicates in order to reduce the boolean-blindness problem). But since this is the only place we use K3 or builderHasParameters, I suppose splitting them out doesn't help too much. Though if we want to try better comparisons between each parameter string, then that'll also probably want K3...

The main issue with inlining the function here is that builderHasParameters can short-circuit its for-loop, thus skipping over the return K3::True; path after the loop. However, this is nested within the for-loop of checkForBuilderWithDefaultPrototype, and since C++ doesn't have labeled-loops like Java, that means some ugliness to capture the "break out of the inner loop and then/also continue the outer loop" semantics. For a boolean comparison between parameter strings, I think I've managed to find a not-too-ugly way to get the same semantics when inlining builderHasParameters. Though for a K3-comparison between parameter strings, inlining that too starts to get quite ugly IMO.

Will post a new version inlining builderHasParameters and sticking with a boolean-comparison, for now. Can revisit the topic if/when we switch to using a K3-comparison.

63

None that I've seen so far, but I haven't run the full test suite yet. I do have some tests which trigger the warning in the expected situations; though they're currently commented out due to issues I discussed in the most recent update. (N.B., the version of this differential you commented on had some logic bugs re when to enter the state where it emits a warning. Which have been fixed in the most recent update)

[wrengr]

mlir/lib/TableGen/AttrOrTypeDef.cpp

63

After running check-mlir and check-mlir-integration I didn't see any warnings or errors. But CMake might've hidden them from me. Is there any special CMake incantation to get the complete stderr logs from mlir-tblgen runs? The only logs I can find are for the autoconfig-like stuff before actually building anything.

[wrengr]

git-clang-format

[Mogball]

mlir/lib/TableGen/AttrOrTypeDef.cpp

32

Drop username

51

Drop const

64

Spell out the auto? Also drop const

71–76

80

llvm::zip?

81

I think checking the number of parameters is robust and will always give useful errors. String matching is a rabbit hole for sure, but I'm not currently aware of anyone relying on implicit conversions, so we can stick with this until someone complains.

I would take Rivers suggestion and check "ends with" or even "contains" but on both the cppType and cppStorageType.

89

102

Drop username. Also, can the check be moved someone else? (Into AttrOrTypeDefGen.cpp) then you can reference the flag and ensure the checks are performed at most once

172

Don't add usernames to FIXMEs or TODOs

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

136

Drop username

mlir/lib/TableGen/AttrOrTypeDef.cpp

 bool AttrOrTypeBuilder::hasInferredContextParameter() const {
   return def->getValueAsBit("hasInferredContextParam");
 }
 
 //===----------------------------------------------------------------------===//
 // 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<StringRef> defParams;
+  defParams.reserve(numDefParams);
+  for (const AttrOrTypeParameter &param : 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 =
       dyn_cast_or_null<llvm::ListInit>(def->getValueInit("builders"));
   if (builderList && !builderList->empty()) {
     for (llvm::Init *init : builderList->getValues()) {
       AttrOrTypeBuilder builder(cast<llvm::DefInit>(init)->getDef(),
                                 def->getLoc());
if (getMnemonic()) {
     }
   } else if (hasCppFormat || hasDeclarativeFormat) {
     PrintFatalError(getLoc(),
                     "'assemblyFormat' or 'hasCustomAssemblyFormat' can only be "
                     "used when 'mnemonic' is set");
   }
   // 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(),
                     "'assemblyFormat' requires 'genAccessors' to be true");
   }
 }
 
 Dialect AttrOrTypeDef::getDialect() const {

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

def InvalidTypeQ : InvalidType<"InvalidTypeQ", "invalid_q"> {
   let assemblyFormat = "custom<Foo>(params)";
 }
 
 def InvalidTypeR : InvalidType<"InvalidTypeR", "invalid_r"> {
   let parameters = (ins "int":$a);
   // CHECK: `struct` can only be used at the top-level context
   let assemblyFormat = "custom<Foo>(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<Test_Dialect, "ValidTypeS"> {
+  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<Test_Dialect, "ValidTypeT"> {
+  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<Test_Dialect, "ValidTypeU"> {
+  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)>];
+}
+// */