[mlir][tblgen] Reverting fatality of assemblyFormat with skipDefaultBuilders=1

[wrengr]

Summary

Per @rriddle, we do not want to require skipDefaultBuilders=0 per se; that is, even though the assemblyFormat-generated parser requires a builder with the same prototype as the default-builder, that prototype could instead be implemented via custom builders. This differential reduces the FatalError introduced in D128555 to a non-fatal Warning instead, so that users can still be informed of the error condition (rather than waiting for the C++ compiler to fail).

[rriddle]

Really appreciated!

mlir/lib/TableGen/AttrOrTypeDef.cpp

86

Can you file an issue for this? I 100% agree this is definitely something we want better support for, but we don't really have any good way of signaling override right now (the only real way we have right now is by checking the builder signature, but that is quite fragile).

[rriddle]

Also want to say thanks for the improved error messages in general! They make a difference.

[wrengr]

mlir/lib/TableGen/AttrOrTypeDef.cpp

86

Will do.

I have another CL in the works which tries to check the custom builders, but as you say it's fragile. The only other feasible option I see is adding a new bit hasCustomDefaultBuilder field (with a better name!!), which causes to generate the declarations but not the definitions (similar to how hasCustomAssemblyFormat relates to assemblyFormat); but that's fragile in a different direction :(

[wrengr]

Filed https://github.com/llvm/llvm-project/issues/56415

[jpienaar]

This is reporting warnings during regular builds (llvm/mlir/test/lib/Dialect/Test/TestAttrDefs.td:227:1: warning: using 'assemblyFormat' with 'skipDefaultBuilders=1' may result in C++ compilation errors) , could we disable the warnings there? I wouldn't want folks to get used to ignorning warnings.

[wrengr]

Yeah, I don't want folks getting used to warnings either! :)

For future readers of this differential:

• Further discussion should take place at https://llvm.org/PR56415 so we can keep things in one place.
• D130102 (should land soon) will remove this warning entirely, pending a better resolution to PR56415
• D129243 (WIP) attempts to detect when some custom builder matches the expected prototype. This is inherently fragile due to using string comparisons as a proxy for type equivalence, but there are some other deep-seated issues which are discussed further at https://reviews.llvm.org/D129243#3663647

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 parser requires default builders to be generated.
+  // 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()) {
-    PrintFatalError(
-        getLoc(),
-        "'assemblyFormat' requires 'skipDefaultBuilders' to be false");
+    PrintWarning(getLoc(),
+                 "using 'assemblyFormat' with 'skipDefaultBuilders=1' may "
+                 "result in C++ compilation errors");
   }
   // Assembly format printer requires accessors to be generated.
   if (hasDeclarativeFormat && !genAccessors()) {
     PrintFatalError(getLoc(),
                     "'assemblyFormat' requires 'genAccessors' to be true");
   }
 }