[MLIR][TableGen] Automatic detection and elimination of redundant methods

[jurahul]

Summary

• Change OpClass new method addition to find and eliminate any existing methods that are made redundant by the newly added method, as well as detect if the newly added method will be redundant and return nullptr in that case.
• To facilitate that, add the notion of resolved and unresolved parameters, where resolved parameters have each parameter type known, so that redundancy checks on methods with same name but different parameter types can be done.
• Eliminate existing code to avoid adding conflicting/redundant build methods and rely on this new mechanism to eliminate conflicting build methods.

Fixes https://bugs.llvm.org/show_bug.cgi?id=47095

[jpienaar]

Nice, looks like good reduction

mlir/include/mlir/TableGen/OpClass.h

56

Nit: Start with capital letter as start of a sentence.

56

Nit: either to the given raw_ostream, or to os (the latter already indicates an explicit parameter and so the given is redundant)

79

Prefer comment on line before parameter for uniformity.

83

Nit: K -> kind to avoid clang-tidy warning (especially if we move these again).

89

Same as above here and below.

108

update this too to follow naming convention (https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide)

187

Why is this one in the header file?

200

Doesn't cast assert too if not true?

246

I feel these top comments don't add anything to the enum name really and I'd be pro eliding them.

249

What does only provide here? E.g., if we had MP_Declaration instead is their ambiguity?

326

Does it get pruned or does it just not get added? Pruning for me implies it was already added.

330

So this adds if not redundant and prunes if this makes an existing one redundant? Struggling to capture that all in one name ... addIfNonRedundantAndMaybePrune doesn't roll off the tongue :) This is fine, unless you have a better idea.

384

Why not use setvector if the insertion ordering is required?

mlir/lib/TableGen/OpClass.cpp

84

How about using interleave method here?

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

110

Is this change related?

[jurahul]

Review feedback

mlir/include/mlir/TableGen/OpClass.h

187

Moved it to cpp.

200

I think it does.

326

It could be implemented that way as well (just add and then prune all redundant). But I have clarified the comment to indicate that its either not added or added.

330

Yes, it's a one shot to preserve the invariant that there are no redundant methods at any point. To perfectly explain the functionality its addIfNotRedundant_Else_AddAndPruneRedundant(). I don't have a better idea. I was thinking addMethoAndPruneRedundant() but I don't think the extra redundant adds anything. I am open to suggestions if others have it.

384

SetVector of unique_ptr<>'s is not supported (results in compile time errors due to copy constructors used).

mlir/lib/TableGen/OpClass.cpp

84

interleave() operates on a container of items, so I saved the tokens into a vector so that I can use the interleave here.

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

110

Yes. There is already another $type_attr here below, so the functions for this one were getting removed as the newly added code thinks they are redundant.

[jurahul]

Rebase

[jurahul]

mlir/include/mlir/TableGen/OpClass.h

384

I looked into this a bit more. SetVector maintains 2 copies of the elements in the set, one in a set and one in a vector for the insertion order. As a result unique_ptr<> cannot be held in a SetVector.

[jpienaar]

LG, thanks

mlir/include/mlir/TableGen/OpClass.h

 
 #ifndef MLIR_TABLEGEN_OPCLASS_H_
 #define MLIR_TABLEGEN_OPCLASS_H_
 
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Support/raw_ostream.h"
 
+#include <set>
 #include <string>
 
 namespace mlir {
 namespace tblgen {
 class FmtObjectBase;
 
+// Class for holding a single parameter of an op's method for C++ code emission.
+class OpMethodParameter {
+public:
+  // Properties (qualifiers) for the parameter.
+  enum Property {
+    PP_None = 0x0,
+    PP_Optional = 0x1,
+  };
+
+  OpMethodParameter(StringRef type, StringRef name, StringRef defaultValue = "",
+                    Property properties = PP_None)
+      : type(type), name(name), defaultValue(defaultValue),
+        properties(properties) {}
+
+  OpMethodParameter(StringRef type, StringRef name, Property property)
+      : OpMethodParameter(type, name, "", property) {}
+
+  // write the parameter as a part of a method declaration to the given `os`.
+  void writeDeclTo(raw_ostream &os) const { writeTo(os, /*emitDefault=*/true); }
+
+  // write the parameter as a part of a method definition to the given `os`
+  void writeDefTo(raw_ostream &os) const { writeTo(os, /*emitDefault=*/false); }
+
+  const std::string &getType() const { return type; }
+  bool hasDefaultValue() const { return !defaultValue.empty(); }
+
+private:
+  void writeTo(raw_ostream &os, bool emitDefault) const;
+
+  std::string type;
+  std::string name;
+  std::string defaultValue;
+  Property properties;
+};
+
+// Base class for holding parameters of an op's method for C++ code emission.
+class OpMethodParameters {
+public:
+  // Discriminator for LLVM-style RTTI.
+  enum ParamsKind {
+    PK_Unresolved, // Separate type and name for each parameter is not known
+    PK_Resolved,   // Each parameter is resolved to a type and name
+  };
+
+  OpMethodParameters(ParamsKind K) : Kind(K) {}
+  virtual ~OpMethodParameters() {}
+
+  // LLVM-style RTTI support.
+  ParamsKind getKind() const { return Kind; }
+
+  // write the parameters as a part of a method declaration to the given `os`.
+  virtual void writeDeclTo(raw_ostream &os) const = 0;
+
+  // write the parameters as a part of a method definition to the given `os`
+  virtual void writeDefTo(raw_ostream &os) const = 0;
+
+  // Factory methods to create the correct type of `OpMethodParameters`
+  // object based on the arguments.
+  static std::unique_ptr<OpMethodParameters> create();
+
+  static std::unique_ptr<OpMethodParameters> create(StringRef params);
+
+  static std::unique_ptr<OpMethodParameters>
+  create(llvm::SmallVectorImpl<OpMethodParameter> &&params);
+
+  static std::unique_ptr<OpMethodParameters>
+  create(StringRef type, StringRef name, StringRef defaultValue = "");
+
+private:
+  const ParamsKind Kind;
+};
+
+// Class for holding unresolved parameters.
+class OpMethodUnresolvedParameters : public OpMethodParameters {
+public:
+  OpMethodUnresolvedParameters(StringRef params)
+      : OpMethodParameters(PK_Unresolved), parameters(params) {}
+
+  // write the parameters as a part of a method declaration to the given `os`.
+  void writeDeclTo(raw_ostream &os) const override;
+
+  // write the parameters as a part of a method definition to the given `os`
+  void writeDefTo(raw_ostream &os) const override;
+
+  // LLVM-style RTTI support.
+  static bool classof(const OpMethodParameters *P) {
+    return P->getKind() == PK_Unresolved;
+  }
+
+private:
+  std::string parameters;
+};
+
+// Class for holding resolved parameters.
+class OpMethodResolvedParameters : public OpMethodParameters {
+public:
+  OpMethodResolvedParameters() : OpMethodParameters(PK_Resolved) {}
+
+  OpMethodResolvedParameters(llvm::SmallVectorImpl<OpMethodParameter> &&params)
+      : OpMethodParameters(PK_Resolved) {
+    for (OpMethodParameter &param : params)
+      parameters.emplace_back(std::move(param));
+  }
+
+  OpMethodResolvedParameters(StringRef type, StringRef name,
+                             StringRef defaultValue)
+      : OpMethodParameters(PK_Resolved) {
+    parameters.emplace_back(type, name, defaultValue);
+  }
+
+  // Returns the number of parameters.
+  size_t getNumParameters() const { return parameters.size(); }
+
+  // Returns if this method makes the `other` method redundant. Note that this
+  // is more than just finding conflicting methods. This method determines if
+  // the 2 set of parameters are conflicting and if so, returns true if this
+  // method has a more general set of parameters that can replace all possible
+  // calls to the `other` method.
+  bool makesRedundant(const OpMethodResolvedParameters &other) const;
+
+  // write the parameters as a part of a method declaration to the given `os`.
+  void writeDeclTo(raw_ostream &os) const override;
+
+  // write the parameters as a part of a method definition to the given `os`
+  void writeDefTo(raw_ostream &os) const override;
+
+  // LLVM-style RTTI support.
+  static bool classof(const OpMethodParameters *P) {
+    return P->getKind() == PK_Resolved;
+  }
+
+private:
+  llvm::SmallVector<OpMethodParameter, 4> parameters;
+};
+
 // Class for holding the signature of an op's method for C++ code emission
 class OpMethodSignature {
 public:
-  OpMethodSignature(StringRef retType, StringRef name, StringRef params);
+  template <typename... Args>
+  OpMethodSignature(StringRef retType, StringRef name, Args &&...args)
+      : returnType(retType), methodName(name),
+        parameters(OpMethodParameters::create(std::forward<Args>(args)...)) {}
+  OpMethodSignature(OpMethodSignature &&) = default;
+
+  // Returns if a method with this signature makes a method with `other`
+  // signature redundant. Only supports resolved parameters. If either
+  // signature is using unresolved parameters, returns false.
+  bool makesRedundant(const OpMethodSignature &other) const {
+    if (methodName != other.methodName)
+      return false;
+    auto *resolved_this =
+        dyn_cast<OpMethodResolvedParameters>(parameters.get());
+    auto *resolved_other =
+        dyn_cast<OpMethodResolvedParameters>(other.parameters.get());
+    if (resolved_this && resolved_other)
+      return resolved_this->makesRedundant(*resolved_other);
+    return false;
+  }
+
+  // Returns the number of parameters (for resolved parameters).
+  size_t getNumParameters() const {
+    assert(isa<OpMethodResolvedParameters>(parameters.get()));
+    return cast<OpMethodResolvedParameters>(parameters.get())
+        ->getNumParameters();
+  }
+
+  // Returns the name of the method.
+  StringRef getName() const { return methodName; }
 
   // Writes the signature as a method declaration to the given `os`.
   void writeDeclTo(raw_ostream &os) const;
+
   // Writes the signature as the start of a method definition to the given `os`.
   // `namePrefix` is the prefix to be prepended to the method name (typically
   // namespaces for qualifying the method definition).
   void writeDefTo(raw_ostream &os, StringRef namePrefix) const;
 
 private:
-  // Returns true if the given C++ `type` ends with '&' or '*', or is empty.
-  static bool elideSpaceAfterType(StringRef type);
-
   std::string returnType;
   std::string methodName;
-  std::string parameters;
+  std::unique_ptr<OpMethodParameters> parameters;
 };
 
 // Class for holding the body of an op's method for C++ code emission
 class OpMethodBody {
 public:
   explicit OpMethodBody(bool declOnly);
 
   OpMethodBody &operator<<(Twine content);

 
 // Class for holding an op's method for C++ code emission
 class OpMethod {
 public:
   // Properties (qualifiers) of class methods. Bitfield is used here to help
   // querying properties.
   enum Property {
     MP_None = 0x0,
-    MP_Static = 0x1,      // Static method
-    MP_Constructor = 0x2, // Constructor
-    MP_Private = 0x4,     // Private method
+    MP_Static = 0x1,                             // Static method
+    MP_Constructor = 0x2,                        // Constructor
+    MP_Private = 0x4,                            // Private method
+    MP_DeclOnly = 0x8,                           // Declaration only
+    MP_StaticDeclOnly = MP_Static | MP_DeclOnly, // Static Declaration only
   };
 
-  OpMethod(StringRef retType, StringRef name, StringRef params,
-           Property property, bool declOnly);
+  template <typename... Args>
+  OpMethod(StringRef retType, StringRef name, Property property, unsigned id,
+           Args &&...args)
+      : properties(property),
+        methodSignature(retType, name, std::forward<Args>(args)...),
+        methodBody(properties & MP_DeclOnly), id(id) {}
+
+  OpMethod(OpMethod &&) = default;
+
   virtual ~OpMethod() = default;
 
   OpMethodBody &body() { return methodBody; }
 
   // Returns true if this is a static method.
   bool isStatic() const { return properties & MP_Static; }
 
   // Returns true if this is a private method.
   bool isPrivate() const { return properties & MP_Private; }
 
+  // Returns the name of this method.
+  StringRef getName() const { return methodSignature.getName(); }
+
+  // Returns the ID for this method
+  unsigned getID() const { return id; }
+
+  // Returns if this method makes the `other` method redundant.
+  bool makesRedundant(const OpMethod &other) const {
+    return methodSignature.makesRedundant(other.methodSignature);
+  }
+
   // Writes the method as a declaration to the given `os`.
   virtual void writeDeclTo(raw_ostream &os) const;
+
   // Writes the method as a definition to the given `os`. `namePrefix` is the
   // prefix to be prepended to the method name (typically namespaces for
   // qualifying the method definition).
   virtual void writeDefTo(raw_ostream &os, StringRef namePrefix) const;
 
 protected:
   Property properties;
-  // Whether this method only contains a declaration.
-  bool isDeclOnly;
   OpMethodSignature methodSignature;
   OpMethodBody methodBody;
+  const unsigned id;
 };
 
 // Class for holding an op's constructor method for C++ code emission.
 class OpConstructor : public OpMethod {
 public:
-  OpConstructor(StringRef retType, StringRef name, StringRef params,
-                Property property, bool declOnly)
-      : OpMethod(retType, name, params, property, declOnly){};
+  template <typename... Args>
+  OpConstructor(StringRef className, Property property, unsigned id,
+                Args &&...args)
+      : OpMethod("", className, property, id, std::forward<Args>(args)...){};
 
   // Add member initializer to constructor initializing `name` with `value`.
   void addMemberInitializer(StringRef name, StringRef value);
 
   // Writes the method as a definition to the given `os`. `namePrefix` is the
   // prefix to be prepended to the method name (typically namespaces for
   // qualifying the method definition).
   void writeDefTo(raw_ostream &os, StringRef namePrefix) const override;
 
 private:
   // Member initializers.
   std::string memberInitializers;
 };
 
 // A class used to emit C++ classes from Tablegen.  Contains a list of public
 // methods and a list of private fields to be emitted.
 class Class {
 public:
   explicit Class(StringRef name);
 
-  // Creates a new method in this class.
-  OpMethod &newMethod(StringRef retType, StringRef name, StringRef params = "",
-                      OpMethod::Property = OpMethod::MP_None,
-                      bool declOnly = false);
-
-  OpConstructor &newConstructor(StringRef params = "", bool declOnly = false);
+  // Adds a new method to this class and prune redundant methods. Returns null
+  // if the method being added gets pruned, else returns a pointer to the added
+  // method. Note that this call may also delete existing methods that are made
+  // redundant by this method to the class.
+  template <typename... Args>
+  OpMethod *addMethodAndPrune(StringRef retType, StringRef name,
+                              OpMethod::Property properties, Args &&...args) {
+    auto newMethod = std::make_unique<OpMethod>(
+        retType, name, properties, nextMethodID++, std::forward<Args>(args)...);
+    return addMethodAndPrune(methods, std::move(newMethod));
+  }
+
+  template <typename... Args>
+  OpMethod *addMethodAndPrune(StringRef retType, StringRef name,
+                              Args &&...args) {
+    return addMethodAndPrune(retType, name, OpMethod::MP_None,
+                             std::forward<Args>(args)...);
+  }
+
+  template <typename... Args>
+  OpConstructor *addConstructorAndPrune(Args &&...args) {
+    auto newConstructor = std::make_unique<OpConstructor>(
+        getClassName(), OpMethod::MP_Constructor, nextMethodID++,
+        std::forward<Args>(args)...);
+    return addMethodAndPrune(constructors, std::move(newConstructor));
+  }
 
   // Creates a new field in this class.
   void newField(StringRef type, StringRef name, StringRef defaultValue = "");
 
   // Writes this op's class as a declaration to the given `os`.
   void writeDeclTo(raw_ostream &os) const;
   // Writes the method definitions in this op's class to the given `os`.
   void writeDefTo(raw_ostream &os) const;
 
   // Returns the C++ class name of the op.
   StringRef getClassName() const { return className; }
 
 protected:
+  // Get a list of all the methods to emit, filtering out hidden ones.
+  void forAllMethods(llvm::function_ref<void(const OpMethod &)> Func) const {
+    using ConsRef = const std::unique_ptr<OpConstructor> &;
+    using MethodRef = const std::unique_ptr<OpMethod> &;
+    llvm::for_each(constructors, [&](ConsRef ptr) { Func(*ptr); });
+    llvm::for_each(methods, [&](MethodRef ptr) { Func(*ptr); });
+  }
+
+  // For deterministic code generation, keep methods sorted in the order in
+  // which they were generated.
+  template <typename MethodTy>
+  struct MethodCompare {
+    bool operator()(const std::unique_ptr<MethodTy> &x,
+                    const std::unique_ptr<MethodTy> &y) {
+      return x->getID() < y->getID();
+    }
+  };
+
+  template <typename MethodTy>
+  using MethodSet =
+      std::set<std::unique_ptr<MethodTy>, MethodCompare<MethodTy>>;
+
+  template <typename MethodTy>
+  MethodTy *addMethodAndPrune(MethodSet<MethodTy> &set,
+                              std::unique_ptr<MethodTy> &&newMethod) {
+    // Check if the new method will be made redundant by existing methods.
+    for (auto &method : set)
+      if (method->makesRedundant(*newMethod))
+        return nullptr;
+
+    // We can add this a method to the set. Prune any existing methods that will
+    // be made redundant by adding this new method. Note that the redundant
+    // check between two methods is more than a conflict check. makesRedundant()
+    // below will check if the new method conflicts with an existing method and
+    // if so, returns true if the new method makes the existing method redundant
+    // because all calls to the existing method can be subsumed by the new
+    // method. So makesRedundant() does a combined job of finding conflicts and
+    // deciding which of the 2 conflicting methods survive.
+    //
+    // Note: llvm::erase_if does not work with sets of std::unique_ptr, so doing
+    // it manually here.
+    for (auto it = set.begin(), end = set.end(); it != end;) {
+      if (newMethod->makesRedundant(*(it->get())))
+        it = set.erase(it);
+      else
+        ++it;
+    }
+
+    MethodTy *ret = newMethod.get();
+    set.insert(std::move(newMethod));
+    return ret;
+  }
+
   std::string className;
-  SmallVector<OpConstructor, 2> constructors;
-  SmallVector<OpMethod, 8> methods;
+  MethodSet<OpConstructor> constructors;
+  MethodSet<OpMethod> methods;
+  unsigned nextMethodID = 0;
   SmallVector<std::string, 4> fields;
 };
 
 // Class for holding an op for C++ code emission
 class OpClass : public Class {
 public:
   explicit OpClass(StringRef name, StringRef extraClassDeclaration = "");
 

mlir/lib/TableGen/OpClass.cpp

 //===- OpClass.cpp - Helper classes for Op C++ code emission --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/TableGen/OpClass.h"
 
 #include "mlir/TableGen/Format.h"
+#include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
+#include <unordered_set>
+
+#define DEBUG_TYPE "mlir-tblgen-opclass"
 
 using namespace mlir;
 using namespace mlir::tblgen;
 
+namespace {
+
+// Returns space to be emitted after the given C++ `type`. return "" if the
+// ends with '&' or '*', or is empty, else returns " ".
+StringRef getSpaceAfterType(StringRef type) {
+  return (type.empty() || type.endswith("&") || type.endswith("*")) ? "" : " ";
+}
+
+} // namespace
+
 //===----------------------------------------------------------------------===//
-// OpMethodSignature definitions
+// OpMethodParameter definitions
 //===----------------------------------------------------------------------===//
 
-OpMethodSignature::OpMethodSignature(StringRef retType, StringRef name,
-                                     StringRef params)
-    : returnType(retType), methodName(name), parameters(params) {}
+void OpMethodParameter::writeTo(raw_ostream &os, bool emitDefault) const {
+  if (properties & PP_Optional)
+    os << "/*optional*/";
+  os << type << getSpaceAfterType(type) << name;
+  if (emitDefault && !defaultValue.empty())
+    os << " = " << defaultValue;
+}
+
+//===----------------------------------------------------------------------===//
+// OpMethodParameters definitions
+//===----------------------------------------------------------------------===//
 
-void OpMethodSignature::writeDeclTo(raw_ostream &os) const {
-  os << returnType << (elideSpaceAfterType(returnType) ? "" : " ") << methodName
-     << "(" << parameters << ")";
+// Factory methods to construct the correct type of `OpMethodParameters`
+// object based on the arguments.
+std::unique_ptr<OpMethodParameters> OpMethodParameters::create() {
+  return std::make_unique<OpMethodResolvedParameters>();
 }
 
-void OpMethodSignature::writeDefTo(raw_ostream &os,
-                                   StringRef namePrefix) const {
+std::unique_ptr<OpMethodParameters>
+OpMethodParameters::create(StringRef params) {
+  return std::make_unique<OpMethodUnresolvedParameters>(params);
+}
+
+std::unique_ptr<OpMethodParameters>
+OpMethodParameters::create(llvm::SmallVectorImpl<OpMethodParameter> &&params) {
+  return std::make_unique<OpMethodResolvedParameters>(std::move(params));
+}
+
+std::unique_ptr<OpMethodParameters>
+OpMethodParameters::create(StringRef type, StringRef name,
+                           StringRef defaultValue) {
+  return std::make_unique<OpMethodResolvedParameters>(type, name, defaultValue);
+}
+
+//===----------------------------------------------------------------------===//
+// OpMethodUnresolvedParameters definitions
+//===----------------------------------------------------------------------===//
+void OpMethodUnresolvedParameters::writeDeclTo(raw_ostream &os) const {
+  os << parameters;
+}
+
+void OpMethodUnresolvedParameters::writeDefTo(raw_ostream &os) const {
   // We need to remove the default values for parameters in method definition.
   // TODO: We are using '=' and ',' as delimiters for parameter
   // initializers. This is incorrect for initializer list with more than one
   // element. Change to a more robust approach.
-  auto removeParamDefaultValue = [](StringRef params) {
-    std::string result;
-    std::pair<StringRef, StringRef> parts;
-    while (!params.empty()) {
-      parts = params.split("=");
-      result.append(result.empty() ? "" : ", ");
-      result += parts.first;
-      params = parts.second.split(",").second;
-    }
-    return result;
-  };
+  StringRef params = parameters;
+  bool First = true;
+    std::pair<StringRef, StringRef> parts = params.split("=");
+    if (!First)
+      os << ", ";
+    os << parts.first;
+    First = false;
+  }
+}
+
+//===----------------------------------------------------------------------===//
+// OpMethodResolvedParameters definitions
+//===----------------------------------------------------------------------===//
+
+// Returns true if a method with these parameters makes a method with parameters
+// `other` redundant. This should return true only if all possible calls to the
+// other method can be replaced by calls to this method.
+bool OpMethodResolvedParameters::makesRedundant(
+    const OpMethodResolvedParameters &other) const {
+  const size_t otherNumParams = other.getNumParameters();
+  const size_t thisNumParams = getNumParameters();
+
+  // All calls to the other method can be replaced this method only if this
+  // method has the same or more arguments number of arguments as the other, and
+  // the common arguments have the same type.
+  if (thisNumParams < otherNumParams)
+    return false;
+  for (int idx : llvm::seq<int>(0, otherNumParams))
+    if (parameters[idx].getType() != other.parameters[idx].getType())
+      return false;
+
+  // If all the common arguments have the same type, we can elide the other
+  // method if this method has the same number of arguments as other or the
+  // first argument after the common ones has a default value (and by C++
+  // requirement, all the later ones will also have a default value).
+  return thisNumParams == otherNumParams ||
+         parameters[otherNumParams].hasDefaultValue();
+}
+
+// write the parameters as a part of a method declaration to the given `os`.
+void OpMethodResolvedParameters::writeDeclTo(raw_ostream &os) const {
+  llvm::interleaveComma(parameters, os, [&](const OpMethodParameter &param) {
+    param.writeDeclTo(os);
+  });
+}
+
+// write the parameters as a part of a method definition to the given `os`
+void OpMethodResolvedParameters::writeDefTo(raw_ostream &os) const {
+  llvm::interleaveComma(parameters, os, [&](const OpMethodParameter &param) {
+    param.writeDefTo(os);
+  });
 
-  os << returnType << (elideSpaceAfterType(returnType) ? "" : " ") << namePrefix
-     << (namePrefix.empty() ? "" : "::") << methodName << "("
-     << removeParamDefaultValue(parameters) << ")";
+//===----------------------------------------------------------------------===//
+// OpMethodSignature definitions
+//===----------------------------------------------------------------------===//
+
+void OpMethodSignature::writeDeclTo(raw_ostream &os) const {
+  os << returnType << getSpaceAfterType(returnType) << methodName << "(";
+  parameters->writeDeclTo(os);
+  os << ")";
 }
 
-bool OpMethodSignature::elideSpaceAfterType(StringRef type) {
-  return type.empty() || type.endswith("&") || type.endswith("*");
+void OpMethodSignature::writeDefTo(raw_ostream &os,
+                                   StringRef namePrefix) const {
+  os << returnType << getSpaceAfterType(returnType) << namePrefix
+     << (namePrefix.empty() ? "" : "::") << methodName << "(";
+  parameters->writeDefTo(os);
+  os << ")";
 }
 
 //===----------------------------------------------------------------------===//
 // OpMethodBody definitions
 //===----------------------------------------------------------------------===//
 
 OpMethodBody::OpMethodBody(bool declOnly) : isEffective(!declOnly) {}
 
void OpMethodBody::writeTo(raw_ostream &os) const {
   if (bodyRef.empty() || bodyRef.back() != '\n')
     os << "\n";
 }
 
 //===----------------------------------------------------------------------===//
 // OpMethod definitions
 //===----------------------------------------------------------------------===//
 
-OpMethod::OpMethod(StringRef retType, StringRef name, StringRef params,
-                   OpMethod::Property property, bool declOnly)
-    : properties(property), isDeclOnly(declOnly),
-      methodSignature(retType, name, params), methodBody(declOnly) {}
 void OpMethod::writeDeclTo(raw_ostream &os) const {
   os.indent(2);
   if (isStatic())
     os << "static ";
   methodSignature.writeDeclTo(os);
   os << ";";
 }
 
 void OpMethod::writeDefTo(raw_ostream &os, StringRef namePrefix) const {
-  if (isDeclOnly)
+  // Do not write definition if the method is decl only.
+  if (properties & MP_DeclOnly)
     return;
-
   methodSignature.writeDefTo(os, namePrefix);
   os << " {\n";
   methodBody.writeTo(os);
   os << "}";
 }
 
 //===----------------------------------------------------------------------===//
 // OpConstructor definitions
 //===----------------------------------------------------------------------===//
 
 void OpConstructor::addMemberInitializer(StringRef name, StringRef value) {
   memberInitializers.append(std::string(llvm::formatv(
       "{0}{1}({2})", memberInitializers.empty() ? " : " : ", ", name, value)));
 }
 
 void OpConstructor::writeDefTo(raw_ostream &os, StringRef namePrefix) const {
-  if (isDeclOnly)
+  // Do not write definition if the method is decl only.
+  if (properties & MP_DeclOnly)
     return;
 
   methodSignature.writeDefTo(os, namePrefix);
   os << " " << memberInitializers << " {\n";
   methodBody.writeTo(os);
   os << "}";
 }
 
 //===----------------------------------------------------------------------===//
 // Class definitions
 //===----------------------------------------------------------------------===//
 
 Class::Class(StringRef name) : className(name) {}
 
-OpMethod &Class::newMethod(StringRef retType, StringRef name, StringRef params,
-                           OpMethod::Property property, bool declOnly) {
-  methods.emplace_back(retType, name, params, property, declOnly);
-  return methods.back();
-}
-
-OpConstructor &Class::newConstructor(StringRef params, bool declOnly) {
-  constructors.emplace_back("", getClassName(), params,
-                            OpMethod::MP_Constructor, declOnly);
-  return constructors.back();
-}
-
 void Class::newField(StringRef type, StringRef name, StringRef defaultValue) {
   std::string varName = formatv("{0} {1}", type, name).str();
   std::string field = defaultValue.empty()
                           ? varName
                           : formatv("{0} = {1}", varName, defaultValue).str();
   fields.push_back(std::move(field));
 }
-
 void Class::writeDeclTo(raw_ostream &os) const {
   bool hasPrivateMethod = false;
   os << "class " << className << " {\n";
   os << "public:\n";
-  for (const auto &method :
-       llvm::concat<const OpMethod>(constructors, methods)) {
+
+  forAllMethods([&](const OpMethod &method) {
     if (!method.isPrivate()) {
       method.writeDeclTo(os);
       os << '\n';
     } else {
       hasPrivateMethod = true;
     }
-  }
+  });
+
   os << '\n';
   os << "private:\n";
   if (hasPrivateMethod) {
-    for (const auto &method :
-         llvm::concat<const OpMethod>(constructors, methods)) {
+    forAllMethods([&](const OpMethod &method) {
       if (method.isPrivate()) {
         method.writeDeclTo(os);
         os << '\n';
       }
-    }
+    });
     os << '\n';
   }
+
   for (const auto &field : fields)
     os.indent(2) << field << ";\n";
   os << "};\n";
 }
 
 void Class::writeDefTo(raw_ostream &os) const {
-  for (const auto &method :
-       llvm::concat<const OpMethod>(constructors, methods)) {
+  forAllMethods([&](const OpMethod &method) {
     method.writeDefTo(os, className);
     os << "\n\n";
-  }
+  });
 }
 
 //===----------------------------------------------------------------------===//
 // OpClass definitions
 //===----------------------------------------------------------------------===//
 
 OpClass::OpClass(StringRef name, StringRef extraClassDeclaration)
     : Class(name), extraClassDeclaration(extraClassDeclaration) {}
 
 void OpClass::addTrait(Twine trait) {
   auto traitStr = trait.str();
   if (traitsSet.insert(traitStr).second)
     traitsVec.push_back(std::move(traitStr));
 }
 
 void OpClass::writeDeclTo(raw_ostream &os) const {
   os << "class " << className << " : public ::mlir::Op<" << className;
   for (const auto &trait : traitsVec)
     os << ", " << trait;
   os << "> {\npublic:\n";
   os << "  using Op::Op;\n";
   os << "  using Adaptor = " << className << "Adaptor;\n";
 
   bool hasPrivateMethod = false;
-  for (const auto &method : methods) {
+  forAllMethods([&](const OpMethod &method) {
     if (!method.isPrivate()) {
       method.writeDeclTo(os);
       os << "\n";
     } else {
       hasPrivateMethod = true;
     }
-  }
+  });
 
   // TODO: Add line control markers to make errors easier to debug.
   if (!extraClassDeclaration.empty())
     os << extraClassDeclaration << "\n";
 
   if (hasPrivateMethod) {
     os << "\nprivate:\n";
-    for (const auto &method : methods) {
+    forAllMethods([&](const OpMethod &method) {
       if (method.isPrivate()) {
         method.writeDeclTo(os);
         os << "\n";
       }
-    }
+    });
   }
 
   os << "};\n";
 }

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

let arguments = (ins
     BoolAttr:$bool_attr,
     I32Attr:$i32_attr,
     I64Attr:$i64_attr,
     F32Attr:$f32_attr,
     F64Attr:$f64_attr,
     StrAttr:$str_attr,
     ElementsAttr:$elements_attr,
     FlatSymbolRefAttr:$function_attr,
-    SomeTypeAttr:$type_attr,
+    SomeTypeAttr:$some_type_attr,
     ArrayAttr:$array_attr,
     TypedArrayAttrBase<SomeAttr, "SomeAttr array">:$some_attr_array,
     TypeAttr:$type_attr
   );
 }
 
 
 // Test common attribute kinds' constraints
 // ---
 
 // DEF-LABEL: BOpAdaptor::verify
 // DEF: if (!((true)))
 // DEF: if (!((tblgen_bool_attr.isa<::mlir::BoolAttr>())))
 // DEF: if (!(((tblgen_i32_attr.isa<::mlir::IntegerAttr>())) && ((tblgen_i32_attr.cast<::mlir::IntegerAttr>().getType().isSignlessInteger(32)))))
 // DEF: if (!(((tblgen_i64_attr.isa<::mlir::IntegerAttr>())) && ((tblgen_i64_attr.cast<::mlir::IntegerAttr>().getType().isSignlessInteger(64)))))
 // DEF: if (!(((tblgen_f32_attr.isa<::mlir::FloatAttr>())) && ((tblgen_f32_attr.cast<::mlir::FloatAttr>().getType().isF32()))))
 // DEF: if (!(((tblgen_f64_attr.isa<::mlir::FloatAttr>())) && ((tblgen_f64_attr.cast<::mlir::FloatAttr>().getType().isF64()))))
 // DEF: if (!((tblgen_str_attr.isa<::mlir::StringAttr>())))
 // DEF: if (!((tblgen_elements_attr.isa<::mlir::ElementsAttr>())))
 // DEF: if (!((tblgen_function_attr.isa<::mlir::FlatSymbolRefAttr>())))
-// DEF: if (!(((tblgen_type_attr.isa<::mlir::TypeAttr>())) && ((tblgen_type_attr.cast<::mlir::TypeAttr>().getValue().isa<SomeType>()))))
+// DEF: if (!(((tblgen_some_type_attr.isa<::mlir::TypeAttr>())) && ((tblgen_some_type_attr.cast<::mlir::TypeAttr>().getValue().isa<SomeType>()))))
 // DEF: if (!((tblgen_array_attr.isa<::mlir::ArrayAttr>())))
 // DEF: if (!(((tblgen_some_attr_array.isa<::mlir::ArrayAttr>())) && (::llvm::all_of(tblgen_some_attr_array.cast<::mlir::ArrayAttr>(), [](::mlir::Attribute attr) { return (some-condition); }))))
 // DEF: if (!(((tblgen_type_attr.isa<::mlir::TypeAttr>())) && ((tblgen_type_attr.cast<::mlir::TypeAttr>().getValue().isa<::mlir::Type>()))))
 
 // Test common attribute kind getters' return types
 // ---
 
 // DEF: ::mlir::Attribute BOp::any_attr()
 // DEF: bool BOp::bool_attr()
 // DEF: uint32_t BOp::i32_attr()
 // DEF: uint64_t BOp::i64_attr()
 // DEF: ::llvm::APFloat BOp::f32_attr()
 // DEF: ::llvm::APFloat BOp::f64_attr()
 // DEF: ::llvm::StringRef BOp::str_attr()
 // DEF: ::mlir::ElementsAttr BOp::elements_attr()
 // DEF: ::llvm::StringRef BOp::function_attr()
-// DEF: SomeType BOp::type_attr()
+// DEF: SomeType BOp::some_type_attr()
 // DEF: ::mlir::ArrayAttr BOp::array_attr()
 // DEF: ::mlir::ArrayAttr BOp::some_attr_array()
 // DEF: ::mlir::Type BOp::type_attr()
 
 // Test building constant values for array attribute kinds
 // ---
 
 def COp : NS_Op<"c_op", []> {

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

 // Test that if the only operand is variadic, we access the first value in the
 // pack to set result type
 // ---
 def OpK : NS_Op<"only_input_is_variadic_with_same_value_type_op", [SameOperandsAndResultType]> {
   let arguments = (ins Variadic<AnyTensor>:$input);
   let results = (outs AnyTensor:$result);
 }
 
-// CHECK-LABEL: OpK::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes )
+// CHECK-LABEL: OpK::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes)
 // CHECK: odsState.addTypes({operands[0].getType()});
 
 // Test with inferred shapes and interleaved with operands/attributes.
 //
 def OpL : NS_Op<"op_with_all_types_constraint",
     [AllTypesMatch<["a", "b"]>]> {
   let arguments = (ins I32Attr:$attr1, AnyType:$a);
   let results = (outs Res<AnyType, "output b", []>:$b);
 }
 
 // CHECK-LABEL: LogicalResult OpL::inferReturnTypes
 // CHECK-NOT: }
 // CHECK: inferredReturnTypes[0] = operands[0].getType();

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

private:
   // type as all results' types.
   void genUseOperandAsResultTypeSeparateParamBuilder();
 
   // Generates the build() method that takes all operands/attributes
   // collectively as one parameter. The generated build() method uses first
   // operand's type as all results' types.
   void genUseOperandAsResultTypeCollectiveParamBuilder();
 
-  // Returns true if the inferred collective param build method should be
-  // generated.
-  bool shouldGenerateInferredTypeCollectiveParamBuilder();
-
   // Generates the build() method that takes aggregate operands/attributes
   // parameters. This build() method uses inferred types as result types.
   // Requires: The type needs to be inferable via InferTypeOpInterface.
   void genInferredTypeCollectiveParamBuilder();
 
   // Generates the build() method that takes each operand/attribute as a
   // stand-alone parameter. The generated build() method uses first attribute's
   // type as all result's types.
enum class AttrParamKind {
     UnwrappedValue, // A raw value without MLIR Attribute wrapper.
   };
 
   // Builds the parameter list for build() method of this op. This method writes
   // to `paramList` the comma-separated parameter list and updates
   // `resultTypeNames` with the names for parameters for specifying result
   // types. The given `typeParamKind` and `attrParamKind` controls how result
   // types and attributes are placed in the parameter list.
-  void buildParamList(std::string &paramList,
+  void buildParamList(llvm::SmallVectorImpl<OpMethodParameter> &paramList,
                       SmallVectorImpl<std::string> &resultTypeNames,
                       TypeParamKind typeParamKind,
                       AttrParamKind attrParamKind = AttrParamKind::WrappedAttr);
 
   // Adds op arguments and regions into operation state for build() methods.
   void genCodeForAddingArgAndRegionForBuilder(OpMethodBody &body,
                                               bool isRawValueAttr = false);
 

 void OpEmitter::emitDef(raw_ostream &os) { opClass.writeDefTo(os); }
 
 void OpEmitter::genAttrGetters() {
   FmtContext fctx;
   fctx.withBuilder("::mlir::Builder(this->getContext())");
 
   // Emit the derived attribute body.
   auto emitDerivedAttr = [&](StringRef name, Attribute attr) {
-    auto &method = opClass.newMethod(attr.getReturnType(), name);
-    auto &body = method.body();
+    auto *method = opClass.addMethodAndPrune(attr.getReturnType(), name);
+    if (!method)
+      return;
+    auto &body = method->body();
     body << "  " << attr.getDerivedCodeBody() << "\n";
   };
 
   // Emit with return type specified.
   auto emitAttrWithReturnType = [&](StringRef name, Attribute attr) {
-    auto &method = opClass.newMethod(attr.getReturnType(), name);
-    auto &body = method.body();
+    auto *method = opClass.addMethodAndPrune(attr.getReturnType(), name);
+    if (!method)
+      return;
+    auto &body = method->body();
     body << "  auto attr = " << name << "Attr();\n";
     if (attr.hasDefaultValue()) {
       // Returns the default value if not set.
       // TODO: this is inefficient, we are recreating the attribute for every
       // call. This should be set instead.
       std::string defaultValue = std::string(
           tgfmt(attr.getConstBuilderTemplate(), &fctx, attr.getDefaultValue()));
       body << "    if (!attr)\n      return "
            << tgfmt(attr.getConvertFromStorageCall(),
                     &fctx.withSelf(defaultValue))
            << ";\n";
     }
     body << "  return "
          << tgfmt(attr.getConvertFromStorageCall(), &fctx.withSelf("attr"))
          << ";\n";
   };
 
   // Generate raw named accessor type. This is a wrapper class that allows
   // referring to the attributes via accessors instead of having to use
   // the string interface for better compile time verification.
   auto emitAttrWithStorageType = [&](StringRef name, Attribute attr) {
-    auto &method =
-        opClass.newMethod(attr.getStorageType(), (name + "Attr").str());
-    auto &body = method.body();
+    auto *method =
+        opClass.addMethodAndPrune(attr.getStorageType(), (name + "Attr").str());
+    if (!method)
+      return;
+    auto &body = method->body();
     body << "  return this->getAttr(\"" << name << "\").";
     if (attr.isOptional() || attr.hasDefaultValue())
       body << "dyn_cast_or_null<";
     else
       body << "cast<";
     body << attr.getStorageType() << ">();";
   };
 
auto derivedAttrs = make_filter_range(op.getAttributes(),
                                           return namedAttr.attr.isDerivedAttr();
                                         });
   if (!derivedAttrs.empty()) {
     opClass.addTrait("::mlir::DerivedAttributeOpInterface::Trait");
     // Generate helper method to query whether a named attribute is a derived
     // attribute. This enables, for example, avoiding adding an attribute that
     // overlaps with a derived attribute.
     {
-      auto &method =
-          opClass.newMethod("bool", "isDerivedAttribute",
-                            "::llvm::StringRef name", OpMethod::MP_Static);
-      auto &body = method.body();
+      auto *method = opClass.addMethodAndPrune("bool", "isDerivedAttribute",
+                                               OpMethod::MP_Static,
+                                               "::llvm::StringRef", "name");
+      auto &body = method->body();
       for (auto namedAttr : derivedAttrs)
         body << "  if (name == \"" << namedAttr.name << "\") return true;\n";
       body << " return false;";
     }
     // Generate method to materialize derived attributes as a DictionaryAttr.
     {
-      OpMethod &method = opClass.newMethod("::mlir::DictionaryAttr",
+      auto *method = opClass.addMethodAndPrune("::mlir::DictionaryAttr",
                                                "materializeDerivedAttributes");
-      auto &body = method.body();
+      auto &body = method->body();
 
       auto nonMaterializable =
           make_filter_range(derivedAttrs, [](const NamedAttribute &namedAttr) {
             return namedAttr.attr.getConvertFromStorageCall().empty();
           });
       if (!nonMaterializable.empty()) {
         std::string attrs;
         llvm::raw_string_ostream os(attrs);
void OpEmitter::genAttrGetters() {
   }
 }
 
 void OpEmitter::genAttrSetters() {
   // Generate raw named setter type. This is a wrapper class that allows setting
   // to the attributes via setters instead of having to use the string interface
   // for better compile time verification.
   auto emitAttrWithStorageType = [&](StringRef name, Attribute attr) {
-    auto &method = opClass.newMethod("void", (name + "Attr").str(),
-                                     (attr.getStorageType() + " attr").str());
-    auto &body = method.body();
+    auto *method = opClass.addMethodAndPrune("void", (name + "Attr").str(),
+                                             attr.getStorageType(), "attr");
+    if (!method)
+      return;
+    auto &body = method->body();
     body << "  this->getOperation()->setAttr(\"" << name << "\", attr);";
   };
 
   for (auto &namedAttr : op.getAttributes()) {
     const auto &name = namedAttr.name;
     const auto &attr = namedAttr.attr;
     if (!attr.isDerivedAttr())
       emitAttrWithStorageType(name, attr);
   }
 }
 
 // Generates the code to compute the start and end index of an operand or result
 // range.
 template <typename RangeT>
 static void
 generateValueRangeStartAndEnd(Class &opClass, StringRef methodName,
                               int numVariadic, int numNonVariadic,
                               StringRef rangeSizeCall, bool hasAttrSegmentSize,
                               StringRef sizeAttrInit, RangeT &&odsValues) {
-  auto &method = opClass.newMethod("std::pair<unsigned, unsigned>", methodName,
-                                   "unsigned index");
-
+  auto *method = opClass.addMethodAndPrune("std::pair<unsigned, unsigned>",
+                                           methodName, "unsigned", "index");
+  if (!method)
+    return;
+  auto &body = method->body();
   if (numVariadic == 0) {
-    method.body() << "  return {index, 1};\n";
+    body << "  return {index, 1};\n";
   } else if (hasAttrSegmentSize) {
-    method.body() << sizeAttrInit << attrSizedSegmentValueRangeCalcCode;
+    body << sizeAttrInit << attrSizedSegmentValueRangeCalcCode;
   } else {
     // Because the op can have arbitrarily interleaved variadic and non-variadic
     // operands, we need to embed a list in the "sink" getter method for
     // calculation at run-time.
     llvm::SmallVector<StringRef, 4> isVariadic;
     isVariadic.reserve(llvm::size(odsValues));
     for (auto &it : odsValues)
       isVariadic.push_back(it.isVariableLength() ? "true" : "false");
     std::string isVariadicList = llvm::join(isVariadic, ", ");
-    method.body() << formatv(sameVariadicSizeValueRangeCalcCode, isVariadicList,
-                             numNonVariadic, numVariadic, rangeSizeCall,
-                             "operand");
+    body << formatv(sameVariadicSizeValueRangeCalcCode, isVariadicList,
+                    numNonVariadic, numVariadic, rangeSizeCall, "operand");
   }
 }
 
 // Generates the named operand getter methods for the given Operator `op` and
 // puts them in `opClass`.  Uses `rangeType` as the return type of getters that
 // return a range of operands (individual operands are `Value ` and each
 // element in the range must also be `Value `); use `rangeBeginCall` to get
 // an iterator to the beginning of the operand range; use `rangeSizeCall` to
static void generateNamedOperandGetters(const Operator &op, Class &opClass,
 
   // First emit a few "sink" getter methods upon which we layer all nicer named
   // getter methods.
   generateValueRangeStartAndEnd(opClass, "getODSOperandIndexAndLength",
                                 numVariadicOperands, numNormalOperands,
                                 rangeSizeCall, attrSizedOperands, sizeAttrInit,
                                 const_cast<Operator &>(op).getOperands());
 
-  auto &m = opClass.newMethod(rangeType, "getODSOperands", "unsigned index");
-  m.body() << formatv(valueRangeReturnCode, rangeBeginCall,
-                      "getODSOperandIndexAndLength(index)");
+  auto *m = opClass.addMethodAndPrune(rangeType, "getODSOperands", "unsigned",
+                                      "index");
+  auto &body = m->body();
+  body << formatv(valueRangeReturnCode, rangeBeginCall,
 
   // Then we emit nicer named getter methods by redirecting to the "sink" getter
   // method.
   for (int i = 0; i != numOperands; ++i) {
     const auto &operand = op.getOperand(i);
     if (operand.name.empty())
       continue;
 
     if (operand.isOptional()) {
-      auto &m = opClass.newMethod("::mlir::Value", operand.name);
-      m.body() << "  auto operands = getODSOperands(" << i << ");\n"
+      m = opClass.addMethodAndPrune("::mlir::Value", operand.name);
+      m->body() << "  auto operands = getODSOperands(" << i << ");\n"
                 << "  return operands.empty() ? Value() : *operands.begin();";
     } else if (operand.isVariadic()) {
-      auto &m = opClass.newMethod(rangeType, operand.name);
-      m.body() << "  return getODSOperands(" << i << ");";
+      m = opClass.addMethodAndPrune(rangeType, operand.name);
+      m->body() << "  return getODSOperands(" << i << ");";
     } else {
-      auto &m = opClass.newMethod("::mlir::Value", operand.name);
-      m.body() << "  return *getODSOperands(" << i << ").begin();";
+      m = opClass.addMethodAndPrune("::mlir::Value", operand.name);
+      m->body() << "  return *getODSOperands(" << i << ").begin();";
     }
   }
 }
 
 void OpEmitter::genNamedOperandGetters() {
   generateNamedOperandGetters(
       op, opClass,
       /*sizeAttrInit=*/
       formatv(opSegmentSizeAttrInitCode, "operand_segment_sizes").str(),
       /*rangeType=*/"::mlir::Operation::operand_range",
       /*rangeBeginCall=*/"getOperation()->operand_begin()",
       /*rangeSizeCall=*/"getOperation()->getNumOperands()",
       /*getOperandCallPattern=*/"getOperation()->getOperand({0})");
 }
 
 void OpEmitter::genNamedOperandSetters() {
   auto *attrSizedOperands = op.getTrait("OpTrait::AttrSizedOperandSegments");
   for (int i = 0, e = op.getNumOperands(); i != e; ++i) {
     const auto &operand = op.getOperand(i);
     if (operand.name.empty())
       continue;
-    auto &m = opClass.newMethod("::mlir::MutableOperandRange",
+    auto *m = opClass.addMethodAndPrune("::mlir::MutableOperandRange",
                                         (operand.name + "Mutable").str());
-    auto &body = m.body();
+    auto &body = m->body();
     body << "  auto range = getODSOperandIndexAndLength(" << i << ");\n"
          << "  return ::mlir::MutableOperandRange(getOperation(), "
             "range.first, range.second";
     if (attrSizedOperands)
       body << ", ::mlir::MutableOperandRange::OperandSegment(" << i
            << "u, *getOperation()->getMutableAttrDict().getNamed("
               "\"operand_segment_sizes\"))";
     body << ");\n";
PrintFatalError(op.getLoc(),
                     "'SameVariadicResultSize' traits");
   }
 
   generateValueRangeStartAndEnd(
       opClass, "getODSResultIndexAndLength", numVariadicResults,
       numNormalResults, "getOperation()->getNumResults()", attrSizedResults,
       formatv(opSegmentSizeAttrInitCode, "result_segment_sizes").str(),
       op.getResults());
-  auto &m = opClass.newMethod("::mlir::Operation::result_range",
-                              "getODSResults", "unsigned index");
-  m.body() << formatv(valueRangeReturnCode, "getOperation()->result_begin()",
+
+  auto *m = opClass.addMethodAndPrune("::mlir::Operation::result_range",
+                                      "getODSResults", "unsigned", "index");
+  m->body() << formatv(valueRangeReturnCode, "getOperation()->result_begin()",
                        "getODSResultIndexAndLength(index)");
 
   for (int i = 0; i != numResults; ++i) {
     const auto &result = op.getResult(i);
     if (result.name.empty())
       continue;
 
     if (result.isOptional()) {
-      auto &m = opClass.newMethod("::mlir::Value", result.name);
-      m.body()
+      m = opClass.addMethodAndPrune("::mlir::Value", result.name);
+      m->body()
           << "  auto results = getODSResults(" << i << ");\n"
           << "  return results.empty() ? ::mlir::Value() : *results.begin();";
     } else if (result.isVariadic()) {
-      auto &m =
-          opClass.newMethod("::mlir::Operation::result_range", result.name);
-      m.body() << "  return getODSResults(" << i << ");";
+      m = opClass.addMethodAndPrune("::mlir::Operation::result_range",
+                                    result.name);
+      m->body() << "  return getODSResults(" << i << ");";
     } else {
-      auto &m = opClass.newMethod("::mlir::Value", result.name);
-      m.body() << "  return *getODSResults(" << i << ").begin();";
+      m = opClass.addMethodAndPrune("::mlir::Value", result.name);
+      m->body() << "  return *getODSResults(" << i << ").begin();";
     }
   }
 }
 
 void OpEmitter::genNamedRegionGetters() {
   unsigned numRegions = op.getNumRegions();
   for (unsigned i = 0; i < numRegions; ++i) {
     const auto &region = op.getRegion(i);
     if (region.name.empty())
       continue;
 
     // Generate the accessors for a varidiadic region.
     if (region.isVariadic()) {
-      auto &m =
-          opClass.newMethod("::mlir::MutableArrayRef<Region>", region.name);
-      m.body() << formatv(
+      auto *m = opClass.addMethodAndPrune("::mlir::MutableArrayRef<Region>",
+                                          region.name);
+      m->body() << formatv(
           "  return this->getOperation()->getRegions().drop_front({0});", i);
       continue;
     }
 
-    auto &m = opClass.newMethod("::mlir::Region &", region.name);
-    m.body() << formatv("  return this->getOperation()->getRegion({0});", i);
+    auto *m = opClass.addMethodAndPrune("::mlir::Region &", region.name);
+    m->body() << formatv("  return this->getOperation()->getRegion({0});", i);
   }
 }
 
 void OpEmitter::genNamedSuccessorGetters() {
   unsigned numSuccessors = op.getNumSuccessors();
   for (unsigned i = 0; i < numSuccessors; ++i) {
     const NamedSuccessor &successor = op.getSuccessor(i);
     if (successor.name.empty())
       continue;
 
     // Generate the accessors for a variadic successor list.
     if (successor.isVariadic()) {
-      auto &m = opClass.newMethod("::mlir::SuccessorRange", successor.name);
-      m.body() << formatv(
+      auto *m =
+          opClass.addMethodAndPrune("::mlir::SuccessorRange", successor.name);
+      m->body() << formatv(
           "  return {std::next(this->getOperation()->successor_begin(), {0}), "
           "this->getOperation()->successor_end()};",
           i);
       continue;
     }
 
-    auto &m = opClass.newMethod("::mlir::Block *", successor.name);
-    m.body() << formatv("  return this->getOperation()->getSuccessor({0});", i);
+    auto *m = opClass.addMethodAndPrune("::mlir::Block *", successor.name);
+    m->body() << formatv("  return this->getOperation()->getSuccessor({0});",
+                         i);
   }
 }
 
 static bool canGenerateUnwrappedBuilder(Operator &op) {
   // If this op does not have native attributes at all, return directly to avoid
   // redefining builders.
   if (op.getNumNativeAttributes() == 0)
     return false;
void OpEmitter::genSeparateArgParamBuilder() {
   attrBuilderType.push_back(AttrParamKind::WrappedAttr);
   if (canGenerateUnwrappedBuilder(op))
     attrBuilderType.push_back(AttrParamKind::UnwrappedValue);
 
   // Emit with separate builders with or without unwrapped attributes and/or
   // inferring result type.
   auto emit = [&](AttrParamKind attrType, TypeParamKind paramKind,
                   bool inferType) {
-    std::string paramList;
+    llvm::SmallVector<OpMethodParameter, 4> paramList;
     llvm::SmallVector<std::string, 4> resultNames;
     buildParamList(paramList, resultNames, paramKind, attrType);
 
-    auto &m =
-        opClass.newMethod("void", "build", paramList, OpMethod::MP_Static);
-    auto &body = m.body();
-
+    auto *m = opClass.addMethodAndPrune("void", "build", OpMethod::MP_Static,
+                                        std::move(paramList));
+    // If the builder is redundant, skip generating the method.
+    if (!m)
+      return;
+    auto &body = m->body();
     genCodeForAddingArgAndRegionForBuilder(
         body, /*isRawValueAttr=*/attrType == AttrParamKind::UnwrappedValue);
 
     // Push all result types to the operation state
 
     if (inferType) {
       // Generate builder that infers type too.
       // TODO: Subsume this with general checking if type can be
case TypeParamKind::Collective: {
              << "u && \"mismatched number of results\");\n";
       body << "  " << builderOpState << ".addTypes(resultTypes);\n";
     }
       return;
     }
     llvm_unreachable("unhandled TypeParamKind");
   };
 
-  // A separate arg param builder method will have a signature which is
-  // ambiguous with the collective params build method (generated in
-  // `genCollectiveParamBuilder` function below) if it has a single
-  // `ArrayReg<Type>` parameter for result types and a single `ArrayRef<Value>`
-  // parameter for the operands, no parameters after that, and the collective
-  // params build method has `attributes` as its last parameter (with
-  // a default value). This will happen when all of the following are true:
-  // 1. [`attributes` as last parameter in collective params build method]:
-  //    getNumVariadicRegions must be 0 (otherwise the collective params build
-  //    method ends with a `numRegions` param, and we don't specify default
-  //    value for attributes).
-  // 2. [single `ArrayRef<Value>` parameter for operands, and no parameters
-  //    after that]: numArgs() must be 1 (if not, each arg gets a separate param
-  //    in the build methods generated here) and the single arg must be a
-  //    non-attribute variadic argument.
-  // 3. [single `ArrayReg<Type>` parameter for result types]:
-  //      3a. paramKind should be Collective, or
-  //      3b. paramKind should be Separate and there should be a single variadic
-  //          result
-  //
-  // In that case, skip generating such ambiguous build methods here.
+  // Some of the build methods generated here may be amiguous, but TableGen's
+  // ambiguous function detection will elide those ones.
   for (auto attrType : attrBuilderType) {
-    // Case 3b above.
-    if (!(op.hasNoVariadicRegions() && op.hasSingleVariadicArg() &&
-          op.hasSingleVariadicResult()))
-      emit(attrType, TypeParamKind::Separate, /*inferType=*/false);
-    if (canInferType(op)) {
-      // When inferType = true, the generated build method does not have
-      // result types. If the op has a single variadic arg, then this build
-      // method will be ambiguous with the collective inferred build method
-      // generated in `genInferredTypeCollectiveParamBuilder`. If we are going
-      // to generate that collective inferred method, suppress generating the
-      // ambiguous build method here.
-      bool buildMethodAmbiguous =
-          op.hasSingleVariadicArg() &&
-          shouldGenerateInferredTypeCollectiveParamBuilder();
-      if (!buildMethodAmbiguous)
-        emit(attrType, TypeParamKind::None, /*inferType=*/true);
-    }
-    // The separate arg + collective param kind method will be:
-    // (a) Same as the separate arg + separate param kind method if there is
-    //     only one variadic result.
-    // (b) Ambiguous with the collective params method under conditions in (3a)
-    //     above.
-    // In either case, skip generating such build method.
-    if (!op.hasSingleVariadicResult() &&
-        !(op.hasNoVariadicRegions() && op.hasSingleVariadicArg()))
-      emit(attrType, TypeParamKind::Collective, /*inferType=*/false);
+    if (canInferType(op))
   }
 }
 
 void OpEmitter::genUseOperandAsResultTypeCollectiveParamBuilder() {
   int numResults = op.getNumResults();
 
   // Signature
-  std::string params =
-      std::string("::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &") +
-      builderOpState +
-      ", ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> "
-      "attributes";
-  if (op.getNumVariadicRegions()) {
-    params += ", unsigned numRegions";
-  } else {
-    // Provide default value for `attributes` since its the last parameter
-    params += " = {}";
-  }
-  auto &m = opClass.newMethod("void", "build", params, OpMethod::MP_Static);
-  auto &body = m.body();
+  llvm::SmallVector<OpMethodParameter, 4> paramList;
+  paramList.emplace_back("::mlir::OpBuilder &", "odsBuilder");
+  paramList.emplace_back("::mlir::OperationState &", builderOpState);
+  paramList.emplace_back("::mlir::ValueRange", "operands");
+  // Provide default value for `attributes` when its the last parameter
+  StringRef attributesDefaultValue = op.getNumVariadicRegions() ? "" : "{}";
+  paramList.emplace_back("::llvm::ArrayRef<::mlir::NamedAttribute>",
+                         "attributes", attributesDefaultValue);
+  if (op.getNumVariadicRegions())
+    paramList.emplace_back("unsigned", "numRegions");
+
+  auto *m = opClass.addMethodAndPrune("void", "build", OpMethod::MP_Static,
+                                      std::move(paramList));
+  // If the builder is redundant, skip generating the method
+  if (!m)
+    return;
+  auto &body = m->body();
 
   // Operands
   body << "  " << builderOpState << ".addOperands(operands);\n";
 
   // Attributes
   body << "  " << builderOpState << ".addAttributes(attributes);\n";
 
   // Create the correct number of regions
   if (int numRegions = op.getNumRegions()) {
     body << llvm::formatv(
         "  for (unsigned i = 0; i != {0}; ++i)\n",
         (op.getNumVariadicRegions() ? "numRegions" : Twine(numRegions)));
     body << "    (void)" << builderOpState << ".addRegion();\n";
   }
 
   // Result types
   SmallVector<std::string, 2> resultTypes(numResults, "operands[0].getType()");
   body << "  " << builderOpState << ".addTypes({"
        << llvm::join(resultTypes, ", ") << "});\n\n";
 }
 
-bool OpEmitter::shouldGenerateInferredTypeCollectiveParamBuilder() {
-  return canInferType(op) && op.getNumSuccessors() == 0;
-}
-
 void OpEmitter::genInferredTypeCollectiveParamBuilder() {
   // TODO: Expand to support regions.
-  std::string params =
-      std::string("::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &") +
-      builderOpState +
-      ", ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> "
-      "attributes = {}";
-  auto &m = opClass.newMethod("void", "build", params, OpMethod::MP_Static);
-  auto &body = m.body();
+  SmallVector<OpMethodParameter, 4> paramList;
+  paramList.emplace_back("::mlir::OpBuilder &", "odsBuilder");
+  paramList.emplace_back("::mlir::OperationState &", builderOpState);
+  paramList.emplace_back("::mlir::ValueRange", "operands");
+  paramList.emplace_back("::llvm::ArrayRef<::mlir::NamedAttribute>",
+                         "attributes", "{}");
+  auto *m = opClass.addMethodAndPrune("void", "build", OpMethod::MP_Static,
+                                      std::move(paramList));
+  // If the builder is redundant, skip generating the method
+  if (!m)
+    return;
+  auto &body = m->body();
 
   int numResults = op.getNumResults();
   int numVariadicResults = op.getNumVariableLengthResults();
   int numNonVariadicResults = numResults - numVariadicResults;
 
   int numOperands = op.getNumOperands();
   int numVariadicOperands = op.getNumVariableLengthOperands();
   int numNonVariadicOperands = numOperands - numVariadicOperands;
void OpEmitter::genInferredTypeCollectiveParamBuilder() {
 
   body << formatv(R"(
     } else
       ::llvm::report_fatal_error("Failed to infer result type(s).");)",
                   opClass.getClassName(), builderOpState);
 }
 
 void OpEmitter::genUseOperandAsResultTypeSeparateParamBuilder() {
-  std::string paramList;
+  llvm::SmallVector<OpMethodParameter, 4> paramList;
   llvm::SmallVector<std::string, 4> resultNames;
   buildParamList(paramList, resultNames, TypeParamKind::None);
 
-  auto &m = opClass.newMethod("void", "build", paramList, OpMethod::MP_Static);
-  genCodeForAddingArgAndRegionForBuilder(m.body());
+  auto *m = opClass.addMethodAndPrune("void", "build", OpMethod::MP_Static,
+                                      std::move(paramList));
+  // If the builder is redundant, skip generating the method
+  if (!m)
+    return;
+  auto &body = m->body();
+  genCodeForAddingArgAndRegionForBuilder(body);
 
   auto numResults = op.getNumResults();
   if (numResults == 0)
     return;
 
   // Push all result types to the operation state
   const char *index = op.getOperand(0).isVariadic() ? ".front()" : "";
   std::string resultType =
       formatv("{0}{1}.getType()", getArgumentName(op, 0), index).str();
-  m.body() << "  " << builderOpState << ".addTypes({" << resultType;
+  body << "  " << builderOpState << ".addTypes({" << resultType;
   for (int i = 1; i != numResults; ++i)
-    m.body() << ", " << resultType;
-  m.body() << "});\n\n";
+    body << ", " << resultType;
+  body << "});\n\n";
 }
 
 void OpEmitter::genUseAttrAsResultTypeBuilder() {
-  std::string params =
-      std::string("::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &") +
-      builderOpState +
-      ", ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> "
-      "attributes";
-  auto &m = opClass.newMethod("void", "build", params, OpMethod::MP_Static);
-  auto &body = m.body();
+  SmallVector<OpMethodParameter, 4> paramList;
+  paramList.emplace_back("::mlir::OpBuilder &", "odsBuilder");
+  paramList.emplace_back("::mlir::OperationState &", builderOpState);
+  paramList.emplace_back("::mlir::ValueRange", "operands");
+  paramList.emplace_back("::llvm::ArrayRef<::mlir::NamedAttribute>",
+                         "attributes", "{}");
+  auto *m = opClass.addMethodAndPrune("void", "build", OpMethod::MP_Static,
+                                      std::move(paramList));
+  // If the builder is redundant, skip generating the method
+  if (!m)
+    return;
+
+  auto &body = m->body();
 
   // Push all result types to the operation state
   std::string resultType;
   const auto &namedAttr = op.getAttribute(0);
 
   body << "  for (auto attr : attributes) {\n";
   body << "    if (attr.first != \"" << namedAttr.name << "\") continue;\n";
   if (namedAttr.attr.isTypeAttr()) {
// 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");
         StringRef body = builderDef->getValueAsString("body");
         bool hasBody = !body.empty();
 
-        auto &method =
-            opClass.newMethod("void", "build", params, OpMethod::MP_Static,
-                              /*declOnly=*/!hasBody);
+        OpMethod::Property properties =
+            hasBody ? OpMethod::MP_Static : OpMethod::MP_StaticDeclOnly;
+        auto *method =
+            opClass.addMethodAndPrune("void", "build", properties, params);
         if (hasBody)
-          method.body() << body;
+          method->body() << body;
       }
     }
     if (op.skipDefaultBuilders()) {
       if (!listInit || listInit->empty())
         PrintFatalError(
             op.getLoc(),
             "default builders are skipped and no custom builders provided");
       return;
void OpEmitter::genBuilder() {
   // 2. one having an aggregated parameter for all result types / operands /
   //    attributes, and
   genCollectiveParamBuilder();
   // 3. one having a stand-alone parameter for each operand and attribute,
   //    use the first operand or attribute's type as all result types
   //    to facilitate different call patterns.
   if (op.getNumVariableLengthResults() == 0) {
     if (op.getTrait("OpTrait::SameOperandsAndResultType")) {
-      // If the operation has a single variadic input, then the build method
-      // generated by `genUseOperandAsResultTypeSeparateParamBuilder` will be
-      // ambiguous with the one generated by
-      // `genUseOperandAsResultTypeCollectiveParamBuilder` (they both will have
-      // a single `ValueRange` argument for operands, and the collective one
-      // will have a `ArrayRef<NamedAttribute>` argument initialized to empty).
-      // Suppress such ambiguous build method.
-      if (!op.hasSingleVariadicArg())
-        genUseOperandAsResultTypeSeparateParamBuilder();
-
-      // The build method generated by the inferred type collective param
-      // builder and one generated here have the same arguments and hence
-      // generating both will be ambiguous. Enable just one of them.
-      if (!shouldGenerateInferredTypeCollectiveParamBuilder())
-        genUseOperandAsResultTypeCollectiveParamBuilder();
     }
     if (op.getTrait("OpTrait::FirstAttrDerivedResultType"))
       genUseAttrAsResultTypeBuilder();
   }
 }
 
 void OpEmitter::genCollectiveParamBuilder() {
   int numResults = op.getNumResults();
   int numVariadicResults = op.getNumVariableLengthResults();
   int numNonVariadicResults = numResults - numVariadicResults;
 
   int numOperands = op.getNumOperands();
   int numVariadicOperands = op.getNumVariableLengthOperands();
   int numNonVariadicOperands = numOperands - numVariadicOperands;
-  // Signature
-  std::string params =
-      std::string("::mlir::OpBuilder &, ::mlir::OperationState &") +
-      builderOpState +
-      ", ::llvm::ArrayRef<::mlir::Type> resultTypes, ::mlir::ValueRange "
-      "operands, "
-      "::llvm::ArrayRef<::mlir::NamedAttribute> attributes";
-  if (op.getNumVariadicRegions()) {
-    params += ", unsigned numRegions";
-  } else {
-    // Provide default value for `attributes` since its the last parameter
-    params += " = {}";
-  }
-  auto &m = opClass.newMethod("void", "build", params, OpMethod::MP_Static);
-  auto &body = m.body();
+
+  SmallVector<OpMethodParameter, 4> paramList;
+  paramList.emplace_back("::mlir::OpBuilder &", "");
+  paramList.emplace_back("::mlir::OperationState &", builderOpState);
+  paramList.emplace_back("::llvm::ArrayRef<::mlir::Type>", "resultTypes");
+  paramList.emplace_back("::mlir::ValueRange", "operands");
+  // Provide default value for `attributes` when its the last parameter
+  StringRef attributesDefaultValue = op.getNumVariadicRegions() ? "" : "{}";
+  paramList.emplace_back("::llvm::ArrayRef<::mlir::NamedAttribute>",
+                         "attributes", attributesDefaultValue);
+  if (op.getNumVariadicRegions())
+    paramList.emplace_back("unsigned", "numRegions");
+
+  auto *m = opClass.addMethodAndPrune("void", "build", OpMethod::MP_Static,
+                                      std::move(paramList));
+  // If the builder is redundant, skip generating the method
+  if (!m)
+    return;
+  auto &body = m->body();
 
   // Operands
   if (numVariadicOperands == 0 || numNonVariadicOperands != 0)
     body << "  assert(operands.size()"
          << (numVariadicOperands != 0 ? " >= " : " == ")
          << numNonVariadicOperands
          << "u && \"mismatched number of parameters\");\n";
   body << "  " << builderOpState << ".addOperands(operands);\n";
void OpEmitter::genCollectiveParamBuilder() {
   if (numVariadicResults == 0 || numNonVariadicResults != 0)
     body << "  assert(resultTypes.size()"
          << (numVariadicResults != 0 ? " >= " : " == ") << numNonVariadicResults
          << "u && \"mismatched number of return types\");\n";
   body << "  " << builderOpState << ".addTypes(resultTypes);\n";
 
   // Generate builder that infers type too.
   // TODO: Expand to handle regions and successors.
-  if (shouldGenerateInferredTypeCollectiveParamBuilder())
+  if (canInferType(op) && op.getNumSuccessors() == 0)
     genInferredTypeCollectiveParamBuilder();
 }
 
-void OpEmitter::buildParamList(std::string &paramList,
+void OpEmitter::buildParamList(SmallVectorImpl<OpMethodParameter> &paramList,
                                SmallVectorImpl<std::string> &resultTypeNames,
                                TypeParamKind typeParamKind,
                                AttrParamKind attrParamKind) {
   resultTypeNames.clear();
   auto numResults = op.getNumResults();
   resultTypeNames.reserve(numResults);
 
-  paramList = "::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &";
-  paramList.append(builderOpState);
+  paramList.emplace_back("::mlir::OpBuilder &", "odsBuilder");
+  paramList.emplace_back("::mlir::OperationState &", builderOpState);
 
   switch (typeParamKind) {
   case TypeParamKind::None:
     break;
   case TypeParamKind::Separate: {
     // Add parameters for all return types
     for (int i = 0; i < numResults; ++i) {
       const auto &result = op.getResult(i);
       std::string resultName = std::string(result.name);
       if (resultName.empty())
         resultName = std::string(formatv("resultType{0}", i));
 
+      StringRef type = result.isVariadic() ? "::llvm::ArrayRef<::mlir::Type>"
+                                           : "::mlir::Type";
+      OpMethodParameter::Property properties = OpMethodParameter::PP_None;
       if (result.isOptional())
-        paramList.append(", /*optional*/::mlir::Type ");
-      else if (result.isVariadic())
-        paramList.append(", ::llvm::ArrayRef<::mlir::Type> ");
-      else
-        paramList.append(", ::mlir::Type ");
-      paramList.append(resultName);
+        properties = OpMethodParameter::PP_Optional;
 
+      paramList.emplace_back(type, resultName, properties);
       resultTypeNames.emplace_back(std::move(resultName));
     }
   } break;
   case TypeParamKind::Collective: {
-    paramList.append(", ::llvm::ArrayRef<::mlir::Type> resultTypes");
+    paramList.emplace_back("::llvm::ArrayRef<::mlir::Type>", "resultTypes");
     resultTypeNames.push_back("resultTypes");
   } break;
   }
 
   // Add parameters for all arguments (operands and attributes).
 
   int numOperands = 0;
   int numAttrs = 0;
for (int i = op.getNumArgs() - 1; i >= 0; --i) {
       defaultValuedAttrStartIndex = i;
     }
   }
 
   for (int i = 0, e = op.getNumArgs(); i < e; ++i) {
     auto argument = op.getArg(i);
     if (argument.is<tblgen::NamedTypeConstraint *>()) {
       const auto &operand = op.getOperand(numOperands);
+      StringRef type =
+          operand.isVariadic() ? "::mlir::ValueRange" : "::mlir::Value";
+      OpMethodParameter::Property properties = OpMethodParameter::PP_None;
       if (operand.isOptional())
-        paramList.append(", /*optional*/::mlir::Value ");
-      else if (operand.isVariadic())
-        paramList.append(", ::mlir::ValueRange ");
-      else
-        paramList.append(", ::mlir::Value ");
-      paramList.append(getArgumentName(op, numOperands));
+        properties = OpMethodParameter::PP_Optional;
+
+      paramList.emplace_back(type, getArgumentName(op, numOperands),
+                             properties);
       ++numOperands;
     } else {
       const auto &namedAttr = op.getAttribute(numAttrs);
       const auto &attr = namedAttr.attr;
-      paramList.append(", ");
 
+      OpMethodParameter::Property properties = OpMethodParameter::PP_None;
       if (attr.isOptional())
-        paramList.append("/*optional*/");
+        properties = OpMethodParameter::PP_Optional;
 
+      StringRef type;
       switch (attrParamKind) {
       case AttrParamKind::WrappedAttr:
-        paramList.append(std::string(attr.getStorageType()));
+        type = attr.getStorageType();
         break;
       case AttrParamKind::UnwrappedValue:
-        if (canUseUnwrappedRawValue(attr)) {
-          paramList.append(std::string(attr.getReturnType()));
-        } else {
-          paramList.append(std::string(attr.getStorageType()));
-        }
+        if (canUseUnwrappedRawValue(attr))
+          type = attr.getReturnType();
+        else
+          type = attr.getStorageType();
         break;
       }
-      paramList.append(" ");
-      paramList.append(std::string(namedAttr.name));
 
+      std::string defaultValue;
       // Attach default value if requested and possible.
       if (attrParamKind == AttrParamKind::UnwrappedValue &&
           i >= defaultValuedAttrStartIndex) {
         bool isString = attr.getReturnType() == "::llvm::StringRef";
-        paramList.append(" = ");
         if (isString)
-          paramList.append("\"");
-        paramList.append(std::string(attr.getDefaultValue()));
+          defaultValue.append("\"");
+        defaultValue += attr.getDefaultValue();
         if (isString)
-          paramList.append("\"");
+          defaultValue.append("\"");
       }
+      paramList.emplace_back(type, namedAttr.name, defaultValue, properties);
       ++numAttrs;
     }
   }
 
   /// Insert parameters for each successor.
   for (const NamedSuccessor &succ : op.getSuccessors()) {
-    paramList += (succ.isVariadic() ? ", ::llvm::ArrayRef<::mlir::Block *> "
-                                    : ", ::mlir::Block *");
-    paramList += succ.name;
+    StringRef type = succ.isVariadic() ? "::llvm::ArrayRef<::mlir::Block *>"
+                                       : "::mlir::Block *";
+    paramList.emplace_back(type, succ.name);
   }
 
   /// Insert parameters for variadic regions.
-  for (const NamedRegion &region : op.getRegions()) {
+  for (const NamedRegion &region : op.getRegions())
     if (region.isVariadic())
-      paramList += llvm::formatv(", unsigned {0}Count", region.name).str();
-  }
+      paramList.emplace_back("unsigned",
+                             llvm::formatv("{0}Count", region.name).str());
 }
 
 void OpEmitter::genCodeForAddingArgAndRegionForBuilder(OpMethodBody &body,
                                                        bool isRawValueAttr) {
   // Push all operands to the result.
   for (int i = 0, e = op.getNumOperands(); i < e; ++i) {
     std::string argName = getArgumentName(op, i);
     if (op.getOperand(i).isOptional())
body << formatv("  {0}.addSuccessors({1});\n", builderOpState,
                     namedSuccessor.name);
   }
 }
 
 void OpEmitter::genCanonicalizerDecls() {
   if (!def.getValueAsBit("hasCanonicalizer"))
     return;
 
-  const char *const params =
-      "::mlir::OwningRewritePatternList &results, ::mlir::MLIRContext *context";
-  opClass.newMethod("void", "getCanonicalizationPatterns", params,
-                    OpMethod::MP_Static, /*declOnly=*/true);
+  SmallVector<OpMethodParameter, 2> paramList;
+  paramList.emplace_back("::mlir::OwningRewritePatternList &", "results");
+  paramList.emplace_back("::mlir::MLIRContext *", "context");
+  opClass.addMethodAndPrune("void", "getCanonicalizationPatterns",
+                            OpMethod::MP_StaticDeclOnly, std::move(paramList));
 }
 
 void OpEmitter::genFolderDecls() {
   bool hasSingleResult =
       op.getNumResults() == 1 && op.getNumVariableLengthResults() == 0;
 
   if (def.getValueAsBit("hasFolder")) {
     if (hasSingleResult) {
-      const char *const params = "::llvm::ArrayRef<::mlir::Attribute> operands";
-      opClass.newMethod("::mlir::OpFoldResult", "fold", params,
-                        OpMethod::MP_None,
-                        /*declOnly=*/true);
+      opClass.addMethodAndPrune(
+          "::mlir::OpFoldResult", "fold", OpMethod::MP_DeclOnly,
+          "::llvm::ArrayRef<::mlir::Attribute>", "operands");
     } else {
-      const char *const params =
-          "::llvm::ArrayRef<::mlir::Attribute> operands, "
-          "::llvm::SmallVectorImpl<::mlir::OpFoldResult> &results";
-      opClass.newMethod("::mlir::LogicalResult", "fold", params,
-                        OpMethod::MP_None,
-                        /*declOnly=*/true);
+      SmallVector<OpMethodParameter, 2> paramList;
+      paramList.emplace_back("::llvm::ArrayRef<::mlir::Attribute>", "operands");
+      paramList.emplace_back("::llvm::SmallVectorImpl<::mlir::OpFoldResult> &",
+                             "results");
+      opClass.addMethodAndPrune("::mlir::LogicalResult", "fold",
+                                OpMethod::MP_DeclOnly, std::move(paramList));
     }
   }
 }
 
 void OpEmitter::genOpInterfaceMethod(const tblgen::InterfaceOpTrait *opTrait) {
   auto interface = opTrait->getOpInterface();
 
   // Get the set of methods that should always be declared.
   auto alwaysDeclaredMethodsVec = opTrait->getAlwaysDeclaredMethods();
   llvm::StringSet<> alwaysDeclaredMethods;
   alwaysDeclaredMethods.insert(alwaysDeclaredMethodsVec.begin(),
                                alwaysDeclaredMethodsVec.end());
 
   for (const InterfaceMethod &method : interface.getMethods()) {
     // Don't declare if the method has a body.
     if (method.getBody())
       continue;
     // Don't declare if the method has a default implementation and the op
     // didn't request that it always be declared.
     if (method.getDefaultImplementation() &&
         !alwaysDeclaredMethods.count(method.getName()))
       continue;
 
-    std::string args;
-    llvm::raw_string_ostream os(args);
-    interleaveComma(method.getArguments(), os,
-                    [&](const InterfaceMethod::Argument &arg) {
-                      os << arg.type << " " << arg.name;
-                    });
-    opClass.newMethod(method.getReturnType(), method.getName(), os.str(),
-                      method.isStatic() ? OpMethod::MP_Static
-                                        : OpMethod::MP_None,
-                      /*declOnly=*/true);
+    SmallVector<OpMethodParameter, 4> paramList;
+    for (const InterfaceMethod::Argument &arg : method.getArguments())
+      paramList.emplace_back(arg.type, arg.name);
+
+    auto properties =
+        method.isStatic() ? OpMethod::MP_StaticDeclOnly : OpMethod::MP_DeclOnly;
+    opClass.addMethodAndPrune(method.getReturnType(), method.getName(),
+                              properties, std::move(paramList));
   }
 }
 
 void OpEmitter::genOpInterfaceMethods() {
   for (const auto &trait : op.getTraits()) {
     if (const auto *opTrait = dyn_cast<tblgen::InterfaceOpTrait>(&trait))
       if (opTrait->shouldDeclareMethods())
         genOpInterfaceMethod(opTrait);
if (op.getArg(i).is<NamedTypeConstraint *>()) {
       ++operandIt;
     }
   }
   /// Results.
   for (unsigned i = 0, e = op.getNumResults(); i != e; ++i)
     resolveDecorators(op.getResultDecorators(i), i, EffectKind::Result);
 
   for (auto &it : interfaceEffects) {
-    auto effectsParam =
-        llvm::formatv("::mlir::SmallVectorImpl<::mlir::SideEffects::"
-                      "EffectInstance<{0}>> &effects",
+    // Generate the 'getEffects' method.
+    std::string type = llvm::formatv("::mlir::SmallVectorImpl<::mlir::"
+                                     "SideEffects::EffectInstance<{0}>> &",
                                      it.first())
                            .str();
-
-    // Generate the 'getEffects' method.
-    auto &getEffects = opClass.newMethod("void", "getEffects", effectsParam);
-    auto &body = getEffects.body();
+    auto *getEffects =
+        opClass.addMethodAndPrune("void", "getEffects", type, "effects");
+    auto &body = getEffects->body();
 
     // Add effect instances for each of the locations marked on the operation.
     for (auto &location : it.second) {
       if (location.kind != EffectKind::Static) {
         body << "  for (::mlir::Value value : getODS"
              << (location.kind == EffectKind::Operand ? "Operands" : "Results")
              << "(" << location.index << "))\n  ";
       }
 
       body << "  effects.emplace_back(" << location.effect.getName()
            << "::get()";
 
       // If the effect isn't static, it has a specific value attached to it.
       if (location.kind != EffectKind::Static)
         body << ", value";
       body << ", " << location.effect.getResource() << "::get());\n";
     }
   }
 }
 
 void OpEmitter::genTypeInterfaceMethods() {
   if (!op.allResultTypesKnown())
     return;
 
-  auto &method = opClass.newMethod(
-      "::mlir::LogicalResult", "inferReturnTypes",
-      "::mlir::MLIRContext* context, "
-      "::llvm::Optional<::mlir::Location> location, "
-      "::mlir::ValueRange operands, ::mlir::DictionaryAttr attributes, "
-      "::mlir::RegionRange regions, "
-      "::llvm::SmallVectorImpl<::mlir::Type>& inferredReturnTypes",
-      OpMethod::MP_Static,
-      /*declOnly=*/false);
-  auto &os = method.body();
-  os << "  inferredReturnTypes.resize(" << op.getNumResults() << ");\n";
+  SmallVector<OpMethodParameter, 4> paramList;
+  paramList.emplace_back("::mlir::MLIRContext *", "context");
+  paramList.emplace_back("::llvm::Optional<::mlir::Location>", "location");
+  paramList.emplace_back("::mlir::ValueRange", "operands");
+  paramList.emplace_back("::mlir::DictionaryAttr", "attributes");
+  paramList.emplace_back("::mlir::RegionRange", "regions");
+  paramList.emplace_back("::llvm::SmallVectorImpl<::mlir::Type>&",
+                         "inferredReturnTypes");
+  auto *method =
+      opClass.addMethodAndPrune("::mlir::LogicalResult", "inferReturnTypes",
+                                OpMethod::MP_Static, std::move(paramList));
+
+  auto &body = method->body();
+  body << "  inferredReturnTypes.resize(" << op.getNumResults() << ");\n";
 
   FmtContext fctx;
   fctx.withBuilder("odsBuilder");
-  os << "  ::mlir::Builder odsBuilder(context);\n";
+  body << "  ::mlir::Builder odsBuilder(context);\n";
 
   auto emitType =
       [&](const tblgen::Operator::ArgOrType &type) -> OpMethodBody & {
     if (type.isArg()) {
       auto argIndex = type.getArg();
       assert(!op.getArg(argIndex).is<NamedAttribute *>());
       auto arg = op.getArgToOperandOrAttribute(argIndex);
       if (arg.kind() == Operator::OperandOrAttribute::Kind::Operand)
-        return os << "operands[" << arg.operandOrAttributeIndex()
+        return body << "operands[" << arg.operandOrAttributeIndex()
                     << "].getType()";
-      return os << "attributes[" << arg.operandOrAttributeIndex()
+      return body << "attributes[" << arg.operandOrAttributeIndex()
                   << "].getType()";
     } else {
-      return os << tgfmt(*type.getType().getBuilderCall(), &fctx);
+      return body << tgfmt(*type.getType().getBuilderCall(), &fctx);
     }
   };
 
   for (int i = 0, e = op.getNumResults(); i != e; ++i) {
-    os << "  inferredReturnTypes[" << i << "] = ";
+    body << "  inferredReturnTypes[" << i << "] = ";
     auto types = op.getSameTypeAsResult(i);
     emitType(types[0]) << ";\n";
     if (types.size() == 1)
       continue;
     // TODO: We could verify equality here, but skipping that for verification.
   }
-  os << "  return success();";
+  body << "  return success();";
 }
 
 void OpEmitter::genParser() {
   if (!hasStringAttribute(def, "parser") ||
       hasStringAttribute(def, "assemblyFormat"))
     return;
 
-  auto &method = opClass.newMethod(
-      "::mlir::ParseResult", "parse",
-      "::mlir::OpAsmParser &parser, ::mlir::OperationState &result",
-      OpMethod::MP_Static);
+  SmallVector<OpMethodParameter, 2> paramList;
+  paramList.emplace_back("::mlir::OpAsmParser &", "parser");
+  paramList.emplace_back("::mlir::OperationState &", "result");
+  auto *method =
+      opClass.addMethodAndPrune("::mlir::ParseResult", "parse",
+                                OpMethod::MP_Static, std::move(paramList));
+
   FmtContext fctx;
   fctx.addSubst("cppClass", opClass.getClassName());
   auto parser = def.getValueAsString("parser").ltrim().rtrim(" \t\v\f\r");
-  method.body() << "  " << tgfmt(parser, &fctx);
+  method->body() << "  " << tgfmt(parser, &fctx);
 }
 
 void OpEmitter::genPrinter() {
   if (hasStringAttribute(def, "assemblyFormat"))
     return;
 
   auto valueInit = def.getValueInit("printer");
   CodeInit *codeInit = dyn_cast<CodeInit>(valueInit);
   if (!codeInit)
     return;
 
-  auto &method = opClass.newMethod("void", "print", "::mlir::OpAsmPrinter &p");
+  auto *method =
+      opClass.addMethodAndPrune("void", "print", "::mlir::OpAsmPrinter &", "p");
   FmtContext fctx;
   fctx.addSubst("cppClass", opClass.getClassName());
   auto printer = codeInit->getValue().ltrim().rtrim(" \t\v\f\r");
-  method.body() << "  " << tgfmt(printer, &fctx);
+  method->body() << "  " << tgfmt(printer, &fctx);
 }
 
 void OpEmitter::genVerifier() {
-  auto &method =
-      opClass.newMethod("::mlir::LogicalResult", "verify", /*params=*/"");
-  auto &body = method.body();
+  auto *method = opClass.addMethodAndPrune("::mlir::LogicalResult", "verify");
+  auto &body = method->body();
   body << "  if (failed(" << op.getAdaptorName()
        << "(*this).verify(this->getLoc()))) "
        << "return failure();\n";
 
   auto *valueInit = def.getValueInit("verifier");
   CodeInit *codeInit = dyn_cast<CodeInit>(valueInit);
   bool hasCustomVerify = codeInit && !codeInit->getValue().empty();
   populateSubstitutions(op, "this->getAttr", "this->getODSOperands",
for (const auto &trait : op.getTraits()) {
     if (auto opTrait = dyn_cast<tblgen::NativeOpTrait>(&trait))
       opClass.addTrait(opTrait->getTrait());
     else if (auto opTrait = dyn_cast<tblgen::InterfaceOpTrait>(&trait))
       opClass.addTrait(opTrait->getTrait());
   }
 }
 
 void OpEmitter::genOpNameGetter() {
-  auto &method = opClass.newMethod("::llvm::StringRef", "getOperationName",
-                                   /*params=*/"", OpMethod::MP_Static);
-  method.body() << "  return \"" << op.getOperationName() << "\";\n";
+  auto *method = opClass.addMethodAndPrune(
+      "::llvm::StringRef", "getOperationName", OpMethod::MP_Static);
+  method->body() << "  return \"" << op.getOperationName() << "\";\n";
 }
 
 void OpEmitter::genOpAsmInterface() {
   // If the user only has one results or specifically added the Asm trait,
   // then don't generate it for them. We specifically only handle multi result
   // operations, because the name of a single result in the common case is not
   // interesting(generally 'result'/'output'/etc.).
   // TODO: We could also add a flag to allow operations to opt in to this
   // generation, even if they only have a single operation.
   int numResults = op.getNumResults();
   if (numResults <= 1 || op.getTrait("::mlir::OpAsmOpInterface::Trait"))
     return;
 
   SmallVector<StringRef, 4> resultNames(numResults);
   for (int i = 0; i != numResults; ++i)
     resultNames[i] = op.getResultName(i);
 
   // Don't add the trait if none of the results have a valid name.
   if (llvm::all_of(resultNames, [](StringRef name) { return name.empty(); }))
     return;
   opClass.addTrait("::mlir::OpAsmOpInterface::Trait");
 
   // Generate the right accessor for the number of results.
-  auto &method = opClass.newMethod("void", "getAsmResultNames",
-                                   "OpAsmSetValueNameFn setNameFn");
-  auto &body = method.body();
+  auto *method = opClass.addMethodAndPrune("void", "getAsmResultNames",
+                                           "OpAsmSetValueNameFn", "setNameFn");
+  auto &body = method->body();
   for (int i = 0; i != numResults; ++i) {
     body << "  auto resultGroup" << i << " = getODSResults(" << i << ");\n"
          << "  if (!llvm::empty(resultGroup" << i << "))\n"
          << "    setNameFn(*resultGroup" << i << ".begin(), \""
          << resultNames[i] << "\");\n";
   }
 }
 

 
 OpOperandAdaptorEmitter::OpOperandAdaptorEmitter(const Operator &op)
     : op(op), adaptor(op.getAdaptorName()) {
   adaptor.newField("::mlir::ValueRange", "odsOperands");
   adaptor.newField("::mlir::DictionaryAttr", "odsAttrs");
   const auto *attrSizedOperands =
       op.getTrait("OpTrait::AttrSizedOperandSegments");
   {
-    auto &constructor = adaptor.newConstructor(
-        attrSizedOperands
-            ? "::mlir::ValueRange values, ::mlir::DictionaryAttr attrs"
-            : "::mlir::ValueRange values, ::mlir::DictionaryAttr attrs = "
-              "nullptr");
-    constructor.addMemberInitializer("odsOperands", "values");
-    constructor.addMemberInitializer("odsAttrs", "attrs");
+    SmallVector<OpMethodParameter, 2> paramList;
+    paramList.emplace_back("::mlir::ValueRange", "values");
+    paramList.emplace_back("::mlir::DictionaryAttr", "attrs",
+                           attrSizedOperands ? "" : "nullptr");
+    auto *constructor = adaptor.addConstructorAndPrune(std::move(paramList));
+
+    constructor->addMemberInitializer("odsOperands", "values");
+    constructor->addMemberInitializer("odsAttrs", "attrs");
   }
 
   {
-    auto &constructor = adaptor.newConstructor(
-        llvm::formatv("{0}& op", op.getCppClassName()).str());
-    constructor.addMemberInitializer("odsOperands",
+    auto *constructor = adaptor.addConstructorAndPrune(
+        llvm::formatv("{0}&", op.getCppClassName()).str(), "op");
+    constructor->addMemberInitializer("odsOperands",
                                       "op.getOperation()->getOperands()");
-    constructor.addMemberInitializer("odsAttrs",
+    constructor->addMemberInitializer("odsAttrs",
                                       "op.getOperation()->getAttrDictionary()");
   }
 
   std::string sizeAttrInit =
       formatv(adapterSegmentSizeAttrInitCode, "operand_segment_sizes");
   generateNamedOperandGetters(op, adaptor, sizeAttrInit,
                               /*rangeType=*/"::mlir::ValueRange",
                               /*rangeBeginCall=*/"odsOperands.begin()",
                               /*rangeSizeCall=*/"odsOperands.size()",
                               /*getOperandCallPattern=*/"odsOperands[{0}]");
 
   FmtContext fctx;
   fctx.withBuilder("::mlir::Builder(odsAttrs.getContext())");
 
   auto emitAttr = [&](StringRef name, Attribute attr) {
-    auto &body = adaptor.newMethod(attr.getStorageType(), name).body();
+    auto &body = adaptor.addMethodAndPrune(attr.getStorageType(), name)->body();
     body << "  assert(odsAttrs && \"no attributes when constructing adapter\");"
          << "\n  " << attr.getStorageType() << " attr = "
          << "odsAttrs.get(\"" << name << "\").";
     if (attr.hasDefaultValue() || attr.isOptional())
       body << "dyn_cast_or_null<";
     else
       body << "cast<";
     body << attr.getStorageType() << ">();\n";
if (!attr.isDerivedAttr())
       emitAttr(name, attr);
   }
 
   // Add verification function.
   addVerification();
 }
 
 void OpOperandAdaptorEmitter::addVerification() {
-  auto &method = adaptor.newMethod("::mlir::LogicalResult", "verify",
-                                   /*params=*/"::mlir::Location loc");
-  auto &body = method.body();
+  auto *method = adaptor.addMethodAndPrune("::mlir::LogicalResult", "verify",
+                                           "::mlir::Location", "loc");
+  auto &body = method->body();
 
   const char *checkAttrSizedValueSegmentsCode = R"(
   {
     auto sizeAttr = odsAttrs.get("{0}").cast<::mlir::DenseIntElementsAttr>();
     auto numElements = sizeAttr.getType().cast<::mlir::ShapedType>().getNumElements();
     if (numElements != {1})
       return emitError(loc, "'{0}' attribute for specifying {2} segments "
                        "must have {1} elements");

mlir/tools/mlir-tblgen/OpFormatGen.cpp

if (auto *attr = dyn_cast<AttributeVariable>(&param)) {
       }
     }
   }
 
   body << "  }\n";
 }
 
 void OperationFormat::genParser(Operator &op, OpClass &opClass) {
-  auto &method = opClass.newMethod(
-      "::mlir::ParseResult", "parse",
-      "::mlir::OpAsmParser &parser, ::mlir::OperationState &result",
-      OpMethod::MP_Static);
-  auto &body = method.body();
+  llvm::SmallVector<OpMethodParameter, 4> paramList;
+  paramList.emplace_back("::mlir::OpAsmParser &", "parser");
+  paramList.emplace_back("::mlir::OperationState &", "result");
+
+  auto *method =
+      opClass.addMethodAndPrune("::mlir::ParseResult", "parse",
+                                OpMethod::MP_Static, std::move(paramList));
+  auto &body = method->body();
 
   // Generate variables to store the operands and type within the format. This
   // allows for referencing these variables in the presence of optional
   // groupings.
   for (auto &element : elements)
     genElementParserStorage(&*element, body);
 
   // A format context used when parsing attributes with buildable types.
if (auto *attr = dyn_cast<AttributeVariable>(element)) {
     genTypeOperandPrinter(dir->getInputs(), body) << ", ";
     genTypeOperandPrinter(dir->getResults(), body) << ");\n";
   } else {
     llvm_unreachable("unknown format element");
   }
 }
 
 void OperationFormat::genPrinter(Operator &op, OpClass &opClass) {
-  auto &method = opClass.newMethod("void", "print", "OpAsmPrinter &p");
-  auto &body = method.body();
+  auto *method =
+      opClass.addMethodAndPrune("void", "print", "OpAsmPrinter &", "p");
+  auto &body = method->body();
 
   // Emit the operation name, trimming the prefix if this is the standard
   // dialect.
   body << "  p << \"";
   std::string opName = op.getOperationName();
   if (op.getDialectName() == "std")
     body << StringRef(opName).drop_front(4);
   else