[mlir] ODS: support TableGen dag objects to specify OpBuilder parameters

[ftynse]

Summary

Historically, custom builder specification in OpBuilder has been accepting the
formal parameter list for the builder method as a raw string containing C++.
While this worked well to connect the signature and the body, this became
problematic when ODS needs to manipulate the parameter list, e.g. to inject
OpBuilder or to trim default values when generating the definition. This has
also become inconsistent with other method declarations, in particular in
interface definitions.

Introduce the possibility to define OpBuilder formal parameters using a
TableGen dag similarly to other methods. Additionally, introduce a mechanism to
declare parameters with default values using an additional class. This
mechanism can be reused in other methods. The string-based builder signature
declaration is deprecated and will be removed after a transition period.

[ftynse]

This has been discussed in the first part of https://llvm.discourse.group/t/rfc-dsl-style-dialect-specific-builders/1823/3.

[ftynse]

Here's an automatic rewriter to change the syntax - https://reviews.llvm.org/D89474. I will update all in-tree users.

[jpienaar]

Nice - well I think this means I can stop migrating the other OpBuilder usage as these will be migrated again :)

mlir/include/mlir/IR/OpBase.td

1985

Seems I reverted this in my change (I had changed including to excluding)

1989

I don't think you ever specify here that Type should be a C++ type or expectation wrt namespace (seeing as changes related to that)

1991

s/has/may have/ ?

1996

Why not use ? and detect if unset instead? You can even avoid specifying these in the <> section and inside do

let = ...

so you can avoid needing to introduce another sentinel and reuse the builtin one (you do need to be more careful and verify that it is set before trying to query it then, but avoids the linkage).

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1156

This is unreachable code

1191

This is unreachable code

1219

Micro nit: does std::string(llvm::formatv(...)) result in nicer formatting?

[lattner]

mlir/docs/OpDefinitions.md

581

This is really cool functionality, I think this is a big step up over the string based thing.

I'd suggest a slightly different approach to this writing - few people know that the (x thing:$foo) syntax is a tablegen "dag" and the writing may come across as scary for people who don't know this stuff.

I'd suggest you keep the first paragraph, then go into the example. Start with a simple example without default arguments (maybe a convenience constructor that takes an integer for the float attr or something), then show that you can add a default argument to it.

This allows you to build the tower of complexity, and explain (in english prose) the concepts as you introduce them.

Thank you for working on this!

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1195

Seems like you could push this into the else branch above, which remove the need for the conditional.

1209

Is this optional aspect here ever used? Seems like all paths return std::string.

[rriddle]

Really great stuff Alex, thanks!

[ftynse]

Address comments.

[ftynse]

Nice - well I think this means I can stop migrating the other OpBuilder usage as these will be migrated again :)

Yeah, your changes nudged be to clean up and propose mine :)

mlir/docs/OpDefinitions.md

581

Great suggestion! I expanded the explanation. There is some text in the introduction that explains relevant TableGen concepts, but it's quite far away from here.

mlir/include/mlir/IR/OpBase.td

1989

Added a description below.

1996

Great idea, thanks!

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

1209

I had it for symmetry with the other case, but as @jpienaar pointed out, it is not necessary in that case either so I removed it.

1219

It does, thanks!

[jpienaar]

Looks good, thanks! An extra failure case for line 1189 would be nice to have.

[ftynse]

Add a test for Tablegen error messages

[ftynse]

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

[jpienaar]

In D89470#2343902, @ftynse wrote:

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

I don't think that works as easily: most error reporting via print fatal error here, this is different from the opt tool where error is returned and propagated up, so we can verify in the driver. I think only think verify brings additionally is to flag unexpected errors, that FileCheck won't. Is there anything else?

[ftynse]

In D89470#2344468, @jpienaar wrote:

In D89470#2343902, @ftynse wrote:

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

I don't think that works as easily: most error reporting via print fatal error here, this is different from the opt tool where error is returned and propagated up, so we can verify in the driver. I think only think verify brings additionally is to flag unexpected errors, that FileCheck won't. Is there anything else?

Splitting the input file into chunks without having to have as many RUN lines as the file has different checks.

[jpienaar]

In D89470#2344601, @ftynse wrote:

In D89470#2344468, @jpienaar wrote:

In D89470#2343902, @ftynse wrote:

An extra failure case for line 1189 would be nice to have.

Yeah, we should also add -verify-diagnostics to Tablegen.

I don't think that works as easily: most error reporting via print fatal error here, this is different from the opt tool where error is returned and propagated up, so we can verify in the driver. I think only think verify brings additionally is to flag unexpected errors, that FileCheck won't. Is there anything else?

Splitting the input file into chunks without having to have as many RUN lines as the file has different checks.

That is the --split-input-file option (unrelated but mostly only used with verifying errors as it really doesn't help/obscures else). There were some discussion on general tool (https://lists.llvm.org/pipermail/llvm-dev/2020-July/143378.html) to do that [well one could do a combination of csplit and bash file for this case I think as we are only piping it into filecheck post but it would get creative soon :) ]

mlir/docs/OpDefinitions.md

 And there may potentially exist other builders depending on the specific op;
 please refer to the
 [generated C++ file](#run-mlir-tblgen-to-see-the-generated-content) for the
 complete list.
 
 #### Custom builder methods
 
 However, if the above cases cannot satisfy all needs, you can define additional
-convenience build methods with `OpBuilder`.
+convenience build methods in the `builders` field as follows.
 
-`OpBuilder` is a class that takes the parameter list and the optional `build()`
-method body. They are separated because we need to generate op declaration and
-definition into separate files. The parameter list should not include `OpBuilder
-&builder, OperationState &state` as they will be inserted automatically and the
-placeholders `$_builder` and `$_state` used. For legacy/to be deprecated reason
-if the `OpBuilder` parameter starts with `OpBuilder` param, then the parameter
-is used. If the `body` is not provided, only the builder declaration will be
-generated; this provides a way to define complicated builders entirely in C++
-files.
+```tablegen
+def MyOp : Op<"my_op", []> {
+  let arguments = (ins F32Attr:$attr);
+
+  let builders = [
+    OpBuilderDAG<(ins "float":$val)>
+  ];
+}
+```
+
+The `builders` field is a list of custom builders that are added to the Op
+class. In this example, we provide a convenience builder that takes a floating
+point value instead of an attribute. The `ins` prefix is common to many function
+declarations in ODS, which use a TableGen [`dag`](#tablegen-syntax). What
+follows is a comma-separated list of types (quoted string) and names prefixed
+with the `$` sign. This will generate the declaration of a builder method that
+looks like:
+
+```c++
+class MyOp : /*...*/ {
+  /*...*/
+  static void build(::mlir::OpBuilder &builder, ::mlir::OperationState &state,
+                    float val);
+};
+```
 
-For example, for the following op:
+Note that the method has two additional leading arguments. These arguments are
+useful to construct the operation. In particular, the method must populate
+`state` with attributes, operands, regions and result types of the operation to
+be constructed. `builder` can be used to construct any IR objects that belong to
+the Op, such as types or nested operations. Since the type and name are
+generated as is in the C++ code, they should be valid C++ constructs for a type
+(in the namespace of the Op) and an identifier (e.g., `class` is not a valid
+identifier).
+
+Implementations of the builder can be provided directly in ODS, using TableGen
+code block as follows.
 
 ```tablegen
 def MyOp : Op<"my_op", []> {
   let arguments = (ins F32Attr:$attr);
 
-  let results = (outs);
+  let builders = [
+    OpBuilderDAG<(ins "float":$val), [{
+      $_state.addAttribute("attr", $_builder.getF32FloatAttr(val));
+    }]>
+  ];
 }
 ```
 
-If we want to define a builder with a default value for the only attribute, we
-can add into `MyOp`:
+The equivalents of `builder` and `state` arguments are available as `$_builder`
+and `$_state` special variables. The named arguments listed in the `ins` part
+are available directly, e.g. `val`. The body of the builder will be generated by
+substituting special variables and should otherwise be valid C++. While there is
+no limitation on the code size, we encourage one to define only short builders
+inline in ODS and put definitions of longer builders in C++ files.
+
+Finally, if some arguments need a default value, they can be defined using
+`CArg` to wrap the type and this value as follows.
 
 ```tablegen
-def MyOp : ... {
-  ...
+def MyOp : Op<"my_op", []> {
+  let arguments = (ins F32Attr:$attr);
 
   let builders = [
-    OpBuilder<"float val = 0.5f", [{
+    OpBuilderDAG<(ins CArg<"float", "0.5f">:$val), [{
       $_state.addAttribute("attr", $_builder.getF32FloatAttr(val));
     }]>
   ];
 }
 ```
 
-The generated builder will look like:
+The generated code will use default value in the declaration, but not in the
+definition, as required by C++.
 
 ```c++
-static void build(OpBuilder &builder, OperationState &state, float val = 0.5f) {
+/* Header file. */
+class MyOp : /*...*/ {
+  /*...*/
+  static void build(::mlir::OpBuilder &builder, ::mlir::OperationState &state,
+                    float val = 0.5f);
+};
+
+/* Source file. */
+MyOp::build(::mlir::OpBuilder &builder, ::mlir::OperationState &state,
+            float val) {
   state.addAttribute("attr", builder.getF32FloatAttr(val));
 }
 ```
 
+**Deprecated:** `OpBuilder` class allows one to specify the custom builder
+signature as a raw string, without separating parameters into different `dag`
+arguments. It also supports leading parameters of `OpBuilder &` and
+`OperationState &` types, which will be used instead of the autogenerated ones
+if present.
+
 ### Custom parser and printer methods
 
 Functions to parse and print the operation's custom assembly form.
 
 ### Custom verifier code
 
 Verification code will be automatically generated for
 [constraints](#constraints) specified on various entities of the op. To

mlir/include/mlir/IR/OpBase.td

 
 //===----------------------------------------------------------------------===//
 // OpInterface definitions
 //===----------------------------------------------------------------------===//
 
 // Marker used to identify the argument list for an op or interface method.
 def ins;
 
+// This class represents a typed argument with optional default value for C
+// function signatures, e.g. builders or methods.
+class CArg<string ty, string value = ""> {
+  string type = ty;
+  string defaultValue = value;
+}
+
 // OpInterfaceTrait corresponds to a specific 'OpInterface' class defined in
 // C++. The purpose to wrap around C++ symbol string with this class is to make
 // interfaces specified for ops in TableGen less alien and more integrated.
 class OpInterfaceTrait<string name, code verifyBody = [{}]>
           : NativeOpTrait<""> {
   let trait = name # "::Trait";
 
   // Specify the body of the verification function. `$_op` will be replaced with

 def outs;
 
 // Marker used to identify the region list for an op.
 def region;
 
 // Marker used to identify the successor list for an op.
 def successor;
 
+// Base class for custom builders. This is a transient class that will go away
+// when the transition to the DAG form of builder declaration is complete.
+// Should not be used directly.
+class OpBuilderBase<dag dp, code b> {
+  string params = ?;
+  dag dagParams = dp;
+  code body = b;
+}
+
 // Class for defining a custom builder.
 //
 // TableGen generates several generic builders for each op by default (see
 // comment in the `Op` class). If the default generated ones cannot cover
 // some use case, custom builders can be defined using instances of this class.
 //
 // The signature of the builder is always
 //
 // ```c++
-// static void build(OpBuilder &builder, OperationState &state,
+// static void build(::mlir::OpBuilder &builder, ::mlir::OperationState &state,
 //                   <other-parameters>...) {
 //   <body>...
 // }
 // ```
 //
-// To define a custom builder, the parameter list (*excluding* the `Builder
-// *builder, OperationState &state` part) and body should be passed in
-// as separate template arguments to this class. This is because we generate
-// op declaration and definition into separate files. If an empty string is
-// passed in for `body`, then *only* the builder declaration will be
-// generated; this provides a way to define complicated builders entirely
-// in C++.
-class OpBuilder<string p, code b = ""> {
-  string params = p;
-  code body = b;
+// To define a custom builder, the parameter list (*excluding* the
+// `OpBuilder &builder, OperationState &state` part) and body should be passed
+// in as separate template arguments to this class. The parameter list is a
+// TableGen DAG with `ins` operation with named arguments, which has either:
+//   - string initializers ("Type":$name) to represent a typed parameter, or
+//   - CArg-typed initializers (CArg<"Type", "default">:$name) to represent a
+//     typed parameter that may have a default value.
+// The type string is used verbatim to produce code and, therefore, must be a
+// valid C++ type. It is used inside the C++ namespace of the parent Op's
+// dialect; explicit namespace qualification like `::mlir` may be necessary if
+// Ops are not placed inside the `mlir` namespace. The default value string is
+// used verbatim to produce code and must be a valid C++ initializer the given
+// type. For example, the following signature specification
+//
+// ```
+// OpBuilderDAG<(ins "int":$integerArg, CArg<"float", "3.0f">:$floatArg)>
+// ```
+//
+// has an integer parameter and a float parameter with a default value.
+//
+// If an empty string is passed in for `body`, then *only* the builder
+// declaration will be generated; this provides a way to define complicated
+// builders entirely in C++.
+class OpBuilderDAG<dag p, code b = ""> : OpBuilderBase<p, b>;
+
+// Deprecated version of OpBuilder that takes the builder signature as string.
+class OpBuilder<string p, code b = ""> : OpBuilderBase<(ins), b> {
+  let params = p;
 }
 
 // A base decorator class that may optionally be added to OpVariables.
 class OpVariableDecorator;
 
 // Class for providing additional information on the variables, i.e. arguments
 // and results, of an operation.
 class OpVariable<Constraint varConstraint, string desc = "",
                  list<OpVariableDecorator> varDecorators = []> {
   // The constraint, either attribute or type, of the argument.
   Constraint constraint = varConstraint;
 
   // A description for the argument.
   string description = desc;
 
   // The list of decorators for this variable, e.g. side effects.
   list<OpVariableDecorator> decorators = varDecorators;
 }
 class Arg<Constraint constraint, string desc = "",
           list<OpVariableDecorator> decorators = []>
class Op<Dialect dialect, string mnemonic, list<OpTrait> props = []> {
   // * where the attributes follow the same declaration order as in the op.
   //
   // ```c++
   // static void build(OpBuilder &, OperationState &odsState,
   //                   TypeRange resultTypes,
   //                   ValueRange operands,
   //                   ArrayRef<NamedAttribute> attributes);
   // ```
-  list<OpBuilder> builders = ?;
+  list<OpBuilderBase> builders = ?;
 
   // Avoid generating default build functions.  Custom builders must be
   // provided.
   bit skipDefaultBuilders = 0;
 
   // Custom parser.
   code parser = ?;
 

mlir/test/mlir-tblgen/op-decl.td

let results = (outs
     I32:$r,
     Variadic<F32>:$s
   );
 
   let regions = (region
     AnyRegion:$someRegion,
     VariadicRegion<AnyRegion>:$someRegions
   );
-  let builders = [OpBuilder<"Value val">];
+  let builders = [OpBuilderDAG<(ins "Value":$val)>,
+                  OpBuilderDAG<(ins CArg<"int", "0">:$integer)>,
+                  OpBuilder<"double deprecatedForm">];
   let parser = [{ foo }];
   let printer = [{ bar }];
   let verifier = [{ baz }];
 
   let hasCanonicalizer = 1;
   let hasFolder = 1;
 
   let extraClassDeclaration = [{

 // CHECK:   ::mlir::Value r();
 // CHECK:   ::mlir::Region &someRegion();
 // CHECK:   ::mlir::MutableArrayRef<Region> someRegions();
 // CHECK:   ::mlir::IntegerAttr attr1Attr()
 // CHECK:   uint32_t attr1();
 // CHECK:   ::mlir::FloatAttr attr2Attr()
 // CHECK:   ::llvm::Optional< ::llvm::APFloat > attr2();
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, Value val);
+// CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, int integer = 0);
+// CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, double deprecatedForm);
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type r, ::mlir::TypeRange s, ::mlir::Value a, ::mlir::ValueRange b, ::mlir::IntegerAttr attr1, /*optional*/::mlir::FloatAttr attr2, unsigned someRegionsCount)
 // CHECK:   static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type r, ::mlir::TypeRange s, ::mlir::Value a, ::mlir::ValueRange b, uint32_t attr1, /*optional*/::mlir::FloatAttr attr2, unsigned someRegionsCount)
 // CHECK:   static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes, unsigned numRegions)
 // CHECK:   static ::mlir::ParseResult parse(::mlir::OpAsmParser &parser, ::mlir::OperationState &result);
 // CHECK:   void print(::mlir::OpAsmPrinter &p);
 // CHECK:   ::mlir::LogicalResult verify();
 // CHECK:   static void getCanonicalizationPatterns(::mlir::OwningRewritePatternList &results, ::mlir::MLIRContext *context);
 // CHECK:   ::mlir::LogicalResult fold(::llvm::ArrayRef<::mlir::Attribute> operands, ::llvm::SmallVectorImpl<::mlir::OpFoldResult> &results);

 // CHECK: static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
 // CHECK: static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
 
 // Check that default builders can be suppressed.
 // ---
 
 def NS_SkipDefaultBuildersOp : NS_Op<"skip_default_builders", []> {
   let skipDefaultBuilders = 1;
-  let builders = [OpBuilder<"Value val">];
+  let builders = [OpBuilderDAG<(ins "Value":$val)>];
 }
 
 // CHECK-LABEL: NS::SkipDefaultBuildersOp declarations
 // CHECK:     class SkipDefaultBuildersOp
 // CHECK-NOT:   static void build(::mlir::Builder
 // CHECK:       static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, Value
 
 // Check leading underscore in op name

mlir/test/mlir-tblgen/op-error.td

+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR1 %s 2>&1 | FileCheck --check-prefix=ERROR1 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR2 %s 2>&1 | FileCheck --check-prefix=ERROR2 %s
+// RUN: not mlir-tblgen -gen-op-decls -I %S/../../include -DERROR3 %s 2>&1 | FileCheck --check-prefix=ERROR3 %s
+
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect : Dialect {
+  let name = "test_dialect";
+}
+
+#ifdef ERROR1
+// ERROR1: error: expected 'ins'
+def OpInsMissing : Op<Test_Dialect, "ins_missing"> {
+  let builders = [
+    OpBuilderDAG<(outs)>
+  ];
+}
+#endif
+
+#ifdef ERROR2
+// ERROR2: error: expected an argument with default value after other arguments with default values
+def OpDefaultValueNotTrailing : Op<Test_Dialect, "default_value"> {
+  let builders = [
+    OpBuilderDAG<(ins CArg<"int", "42">, "int")>
+  ];
+}
+#endif
+
+#ifdef ERROR3
+// ERROR3: error: expected an argument with default value after other arguments with default values
+def OpDefaultValueNotTrailing : Op<Test_Dialect, "default_value"> {
+  let builders = [
+    OpBuilderDAG<(ins CArg<"int", "42">, CArg<"int">)>
+  ];
+}
+#endif

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

void OpEmitter::genUseAttrAsResultTypeBuilder() {
 
   // Result types
   SmallVector<std::string, 2> resultTypes(op.getNumResults(), resultType);
   body << "    " << builderOpState << ".addTypes({"
        << llvm::join(resultTypes, ", ") << "});\n";
   body << "  }\n";
 }
 
+/// Returns a signature of the builder as defined by a dag-typed initializer.
+/// Updates the context `fctx` to enable replacement of $_builder and $_state
+/// in the body. Reports errors at `loc`.
+static std::string builderSignatureFromDAG(const DagInit *init,
+                                           ArrayRef<llvm::SMLoc> loc,
+                                           FmtContext &fctx) {
+  auto *defInit = dyn_cast<DefInit>(init->getOperator());
+  if (!defInit || !defInit->getDef()->getName().equals("ins"))
+    PrintFatalError(loc, "expected 'ins' in builders");
+
+  // Inject builder and state arguments.
+  llvm::SmallVector<std::string, 8> arguments;
+  arguments.reserve(init->getNumArgs() + 2);
+  arguments.push_back(llvm::formatv("::mlir::OpBuilder &{0}", builder).str());
+  arguments.push_back(
+      llvm::formatv("::mlir::OperationState &{0}", builderOpState).str());
+
+  // Accept either a StringInit or a DefInit with two string values as dag
+  // arguments. The former corresponds to the type, the latter to the type and
+  // the default value. Similarly to C++, once an argument with a default value
+  // is detected, the following arguments must have default values as well.
+  bool seenDefaultValue = false;
+  for (unsigned i = 0, e = init->getNumArgs(); i < e; ++i) {
+    // If no name is provided, generate one.
+    StringInit *argName = init->getArgName(i);
+    std::string name =
+        argName ? argName->getValue().str() : "odsArg" + std::to_string(i);
+
+    Init *argInit = init->getArg(i);
+    StringRef type;
+    std::string defaultValue;
+    if (StringInit *strType = dyn_cast<StringInit>(argInit)) {
+      type = strType->getValue();
+    } else {
+      const Record *typeAndDefaultValue = cast<DefInit>(argInit)->getDef();
+      type = typeAndDefaultValue->getValueAsString("type");
+      StringRef defaultValueRef =
+          typeAndDefaultValue->getValueAsString("defaultValue");
+      if (!defaultValueRef.empty()) {
+        seenDefaultValue = true;
+        defaultValue = llvm::formatv(" = {0}", defaultValueRef).str();
+      }
+    }
+    if (seenDefaultValue && defaultValue.empty())
+      PrintFatalError(loc,
+                      "expected an argument with default value after other "
+                      "arguments with default values");
+    arguments.push_back(
+        llvm::formatv("{0} {1}{2}", type, name, defaultValue).str());
+  }
+
+  fctx.withBuilder(builder);
+  fctx.addSubst("_state", builderOpState);
+
+  return llvm::join(arguments, ", ");
+}
+
+// Returns a signature fo the builder as defined by a string initializer,
+// optionally injecting the builder and state arguments.
+// TODO: to be removed after the transition is complete.
+static std::string builderSignatureFromString(StringRef params,
+                                              FmtContext &fctx) {
+  bool skipParamGen = params.startswith("OpBuilder") ||
+                      params.startswith("mlir::OpBuilder") ||
+                      params.startswith("::mlir::OpBuilder");
+  if (skipParamGen)
+    return params.str();
+
+  fctx.withBuilder(builder);
+  fctx.addSubst("_state", builderOpState);
+  return std::string(llvm::formatv("::mlir::OpBuilder &{0}, "
+                                   "::mlir::OperationState &{1}{2}{3}",
+                                   builder, builderOpState,
+                                   params.empty() ? "" : ", ", params));
+}
+
 void OpEmitter::genBuilder() {
   // Handle custom builders if provided.
   // TODO: Create wrapper class for OpBuilder to hide the native
   // TableGen API calls here.
   {
     auto *listInit = dyn_cast_or_null<ListInit>(def.getValueInit("builders"));
     if (listInit) {
       for (Init *init : listInit->getValues()) {
         Record *builderDef = cast<DefInit>(init)->getDef();
-        StringRef params = builderDef->getValueAsString("params").trim();
-        // TODO: Remove this and just generate the builder/state always.
-        bool skipParamGen = params.startswith("OpBuilder") ||
-                            params.startswith("mlir::OpBuilder") ||
-                            params.startswith("::mlir::OpBuilder");
+        llvm::Optional<StringRef> params =
+            builderDef->getValueAsOptionalString("params");
+        FmtContext fctx;
+        std::string paramStr =
+            params.hasValue() ? builderSignatureFromString(params->trim(), fctx)
+                              : builderSignatureFromDAG(
+                                    builderDef->getValueAsDag("dagParams"),
+                                    op.getLoc(), fctx);
+
         StringRef body = builderDef->getValueAsString("body");
         bool hasBody = !body.empty();
-
         OpMethod::Property properties =
             hasBody ? OpMethod::MP_Static : OpMethod::MP_StaticDeclaration;
-        std::string paramStr =
-            skipParamGen ? params.str()
-                         : llvm::formatv("::mlir::OpBuilder &{0}, "
-                                         "::mlir::OperationState &{1}{2}{3}",
-                                         builder, builderOpState,
-                                         params.empty() ? "" : ", ", params)
-                               .str();
         auto *method =
             opClass.addMethodAndPrune("void", "build", properties, paramStr);
-        if (hasBody) {
-          if (skipParamGen) {
-            method->body() << body;
-          } else {
-            FmtContext fctx;
-            fctx.withBuilder(builder);
-            fctx.addSubst("_state", builderOpState);
-            method->body() << tgfmt(body, &fctx);
-          }
-        }
-      }
-    }
+        if (hasBody)
     if (op.skipDefaultBuilders()) {
       if (!listInit || listInit->empty())
         PrintFatalError(
             op.getLoc(),
             "default builders are skipped and no custom builders provided");
       return;
     }
   }