[mlir][ods] Cleanup of handling Op vs OpAdaptor

[Mogball]

Summary

In preparation for implementation subrange lookup on attributes.

Depends on D113039

[jpienaar]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

231

Why do we need to pass in isVariadic here? (don't know we know this from the index + Operator)

[Mogball]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

231

True

[Mogball]

Don't pass in isVariadic

[Chia-hungDuan]

Looks good!

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

196–201

Not sure do we need this? I thought the operator overloading down below is enough?

456–463

While seeing auto &op = emit.getOp();, I may consider passing Operator and create the OpOrAdaptorHelper locally because it's more like a helper function for local codegen. (The isOp can be an argument as well)

1004

I think we can keep the type here. the getGetterName doesn't tell it returns StringRef or std::string

1092

ditto

2091–2102

How about early exit for !type.isArg()

2154

I'm thinking if there's a better name which tells its a helper class. Like a pure helper or opAccessor?

[Mogball]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

196–201

Sometimes we need formatv(...).str() because the results of the format need to be stored and, for example, passed into a method signature.

456–463

From my perspective, it looks cleaner when these huge functions have fewer arguments, and since the helper also contains the gen info (the Operator and isOp) it's easier to just pass it in instead of passing all the individual arguments everywhere and creating the helper everywhere.

[jpienaar]

Looks good, thanks (it seems a couple of done items [re: type] was not done, perhaps just missing in the upload)

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

187

isOp confused me a bit as you are passing in an Operator called op. WDYT about something like emitForOp or something like that?

254

Nit: new line in between for consistency

[Chia-hungDuan]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

196–201

I think the Formatter is used to build the string in formatv() with stream operator and the conversion to str() is done by formatv_object. Does formatv(...).str() need to invoke the Formatter::str()?

456–463

I think for this case it'll be 3 arguments and it seems fine and The creation of helper is not a noticeable cost. I think either ways should be fine. I don't have a strong opinion here. Let's just stick on your plan!

[Mogball]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

196–201

No, but sometimes I need to invoke emitHelper.getAttr("attr_name").str() which is calling this function. Otherwise I'd need to call formatv("{0}", emitHelper.getAttr("attr_name")).str()

456–463

It's just messier passing in tons of arguments into every function.

[Mogball]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

456–463

The example I have in my head is something like this:

struct HelperInfo {
  int a;
  bool bunch;
  std::string of;
  raw_ostream &info;

  void do_something_with_info();
};

// Instead of:
static void genFunction(int a, bool bunch, std::string of, raw_ostream &info) {
  HelperInfo helper{a, bunch, of, info};
  helper.do_something_with_info();
}

// Do:
static void genFunction(HelperInfo &helper) {
  helper.do_something_with_info();
}

Especially when you have 3, 4, or more functions that need to accept all these arguments and construct the helper object.

[Mogball]

Rebase, and review comments

[Chia-hungDuan]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

196–201

With this use case then I got it! Thanks

456–463

Sorry, I didn't say it clear. To construct a OpOrAdpatorHelper you need two arguments. I was thinking we could pass the two arguments instead of passing the constrcuted OpOrAdaptorHelper. Thus the number of arguments would increase only 1, right?

I think whether we should pack the arguments it depends on the case. Too many arguments may be a warning but it may be not always the bad thing. (I know you just want to give a quick example)

Take this function as an example, populateSubstitutions, I would expect I call this function it'll do the substitutions of attributes, operands and results and I only need to tell it what operation I want it to work with.

But now, I may also need to know what data structure the function would like to use and this data structure is only used in the function. If the data structure contains some information is used across different functions like,

Foo data
Init(&data);
DoSomething(&data);
DoIfDataHasBlabla(&data);

Then I will say it's fine. But OpOrAdapter is more like a pure function wrapper here. It doesn't convey many data nor handle any logic. It's more like an internal use only structure by certain functions.

We have packed those boilerplate and it does look cleaner now. Thus I was wondering maybe we don't need to pass the helper.

It always has pros and cons in between designs so I just give my opinion over this. Again, I'm fine with the current design.

[Chia-hungDuan]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

744–745

Just curious, is this style done by the formatter? The "?:" operator part

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

 // DEF:      auto tblgen_cAttr = odsAttrs.get("cAttr");
 // DEF-NEXT: if (tblgen_cAttr && !((some-condition)))
 // DEF-NEXT:   return emitError(loc, "'test.a_op' op ""attribute 'cAttr' failed to satisfy constraint: some attribute kind");
 
 // Test getter methods
 // ---
 
 // DEF:      some-attr-kind AOp::aAttrAttr()
-// DEF-NEXT:   (*this)->getAttr(aAttrAttrName()).template cast<some-attr-kind>()
+// DEF-NEXT:   (*this)->getAttr(aAttrAttrName()).cast<some-attr-kind>()
 // DEF:      some-return-type AOp::aAttr() {
 // DEF-NEXT:   auto attr = aAttrAttr()
 // DEF-NEXT:   return attr.some-convert-from-storage();
 
 // DEF:      some-attr-kind AOp::bAttrAttr()
-// DEF-NEXT:   return (*this)->getAttr(bAttrAttrName()).template dyn_cast_or_null<some-attr-kind>()
+// DEF-NEXT:   return (*this)->getAttr(bAttrAttrName()).dyn_cast_or_null<some-attr-kind>()
 // DEF:      some-return-type AOp::bAttr() {
 // DEF-NEXT:   auto attr = bAttrAttr();
 // DEF-NEXT:   if (!attr)
 // DEF-NEXT:       return some-const-builder-call(::mlir::Builder((*this)->getContext()), 4.2).some-convert-from-storage();
 // DEF-NEXT:   return attr.some-convert-from-storage();
 
 // DEF:      some-attr-kind AOp::cAttrAttr()
-// DEF-NEXT:   return (*this)->getAttr(cAttrAttrName()).template dyn_cast_or_null<some-attr-kind>()
+// DEF-NEXT:   return (*this)->getAttr(cAttrAttrName()).dyn_cast_or_null<some-attr-kind>()
 // DEF:      ::llvm::Optional<some-return-type> AOp::cAttr() {
 // DEF-NEXT:   auto attr = cAttrAttr()
 // DEF-NEXT:   return attr ? ::llvm::Optional<some-return-type>(attr.some-convert-from-storage()) : (::llvm::None);
 
 // Test setter methods
 // ---
 
 // DEF:      void AOp::aAttrAttr(some-attr-kind attr) {

 // DEF:      auto tblgen_cAttr = odsAttrs.get("cAttr");
 // DEF-NEXT: if (tblgen_cAttr && !((some-condition)))
 // DEF-NEXT:   return emitError(loc, "'test2.a_get_op' op ""attribute 'cAttr' failed to satisfy constraint: some attribute kind");
 
 // Test getter methods
 // ---
 
 // DEF:      some-attr-kind AgetOp::getAAttrAttr()
-// DEF-NEXT:   (*this)->getAttr(getAAttrAttrName()).template cast<some-attr-kind>()
+// DEF-NEXT:   (*this)->getAttr(getAAttrAttrName()).cast<some-attr-kind>()
 // DEF:      some-return-type AgetOp::getAAttr() {
 // DEF-NEXT:   auto attr = getAAttrAttr()
 // DEF-NEXT:   return attr.some-convert-from-storage();
 
 // DEF:      some-attr-kind AgetOp::getBAttrAttr()
-// DEF-NEXT:   return (*this)->getAttr(getBAttrAttrName()).template dyn_cast_or_null<some-attr-kind>()
+// DEF-NEXT:   return (*this)->getAttr(getBAttrAttrName()).dyn_cast_or_null<some-attr-kind>()
 // DEF:      some-return-type AgetOp::getBAttr() {
 // DEF-NEXT:   auto attr = getBAttrAttr();
 // DEF-NEXT:   if (!attr)
 // DEF-NEXT:       return some-const-builder-call(::mlir::Builder((*this)->getContext()), 4.2).some-convert-from-storage();
 // DEF-NEXT:   return attr.some-convert-from-storage();
 
 // DEF:      some-attr-kind AgetOp::getCAttrAttr()
-// DEF-NEXT:   return (*this)->getAttr(getCAttrAttrName()).template dyn_cast_or_null<some-attr-kind>()
+// DEF-NEXT:   return (*this)->getAttr(getCAttrAttrName()).dyn_cast_or_null<some-attr-kind>()
 // DEF:      ::llvm::Optional<some-return-type> AgetOp::getCAttr() {
 // DEF-NEXT:   auto attr = getCAttrAttr()
 // DEF-NEXT:   return attr ? ::llvm::Optional<some-return-type>(attr.some-convert-from-storage()) : (::llvm::None);
 
 // Test setter methods
 // ---
 
 // DEF:      void AgetOp::setAAttrAttr(some-attr-kind attr) {

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

 using namespace mlir;
 using namespace mlir::tblgen;
 
 static const char *const tblgenNamePrefix = "tblgen_";
 static const char *const generatedArgName = "odsArg";
 static const char *const odsBuilder = "odsBuilder";
 static const char *const builderOpState = "odsState";
 
-// Code for OpAdaptors to lookup an attribute using strings on the provided
-// DictionaryAttr.
-//
-// {0}: The attribute name.
-static const char *const adaptorGetAttr = "odsAttrs.get(\"{0}\")";
-
-// Code for Ops to lookup an attribute using the cached identifier.
+// Code for an Op to lookup an attribute. Uses cached identifiers.
 //
 // {0}: The attribute's getter name.
 static const char *const opGetAttr = "(*this)->getAttr({0}AttrName())";
 
 // The logic to calculate the actual value range for a declared operand/result
 // of an op with variadic operands/results. Note that this logic is not for
 // general use; it assumes all variadic operands/results must have the same
 // number of values.
 //
 // {0}: The list of whether each declared operand/result is variadic.
 // {1}: The total number of non-variadic operands/results.
 // {2}: The total number of variadic operands/results.
 // {3}: The total number of actual values.
 // {4}: "operand" or "result".
-const char *sameVariadicSizeValueRangeCalcCode = R"(
+static const char *const sameVariadicSizeValueRangeCalcCode = R"(
   bool isVariadic[] = {{{0}};
   int prevVariadicCount = 0;
   for (unsigned i = 0; i < index; ++i)
     if (isVariadic[i]) ++prevVariadicCount;
 
   // Calculate how many dynamic values a static variadic {4} corresponds to.
   // This assumes all static variadic {4}s have the same dynamic value count.
   int variadicSize = ({3} - {1}) / {2};
   // `index` passed in as the parameter is the static index which counts each
   // {4} (variadic or not) as size 1. So here for each previous static variadic
   // {4}, we need to offset by (variadicSize - 1) to get where the dynamic
   // value pack for this static {4} starts.
   int start = index + (variadicSize - 1) * prevVariadicCount;
   int size = isVariadic[index] ? variadicSize : 1;
   return {{start, size};
 )";
 
 // The logic to calculate the actual value range for a declared operand/result
 // of an op with variadic operands/results. Note that this logic is assumes
 // the op has an attribute specifying the size of each operand/result segment
 // (variadic or not).
 //
 // {0}: The name of the attribute specifying the segment sizes.
-const char *adapterSegmentSizeAttrInitCode = R"(
+static const char *const adapterSegmentSizeAttrInitCode = R"(
   assert(odsAttrs && "missing segment size attribute for op");
   auto sizeAttr = odsAttrs.get("{0}").cast<::mlir::DenseIntElementsAttr>();
 )";
-const char *opSegmentSizeAttrInitCode = R"(
-  auto sizeAttr = (*this)->getAttr({0}).cast<::mlir::DenseIntElementsAttr>();
+static const char *const opSegmentSizeAttrInitCode = R"(
+  auto sizeAttr =
+      (*this)->getAttr({0}AttrName()).cast<::mlir::DenseIntElementsAttr>();
 )";
-const char *attrSizedSegmentValueRangeCalcCode = R"(
+static const char *const attrSizedSegmentValueRangeCalcCode = R"(
   const uint32_t *sizeAttrValueIt = &*sizeAttr.value_begin<uint32_t>();
   if (sizeAttr.isSplat())
     return {*sizeAttrValueIt * index, *sizeAttrValueIt};
 
   unsigned start = 0;
   for (unsigned i = 0; i < index; ++i)
     start += sizeAttrValueIt[i];
   return {start, sizeAttrValueIt[index]};
 )";
 // The logic to calculate the actual value range for a declared operand
 // of an op with variadic of variadic operands within the OpAdaptor.
 //
 // {0}: The name of the segment attribute.
 // {1}: The index of the main operand.
-const char *variadicOfVariadicAdaptorCalcCode = R"(
+static const char *const variadicOfVariadicAdaptorCalcCode = R"(
   auto tblgenTmpOperands = getODSOperands({1});
   auto sizeAttrValues = {0}().getValues<uint32_t>();
   auto sizeAttrIt = sizeAttrValues.begin();
 
   ::llvm::SmallVector<::mlir::ValueRange> tblgenTmpOperandGroups;
   for (int i = 0, e = ::llvm::size(sizeAttrValues); i < e; ++i, ++sizeAttrIt) {{
     tblgenTmpOperandGroups.push_back(tblgenTmpOperands.take_front(*sizeAttrIt));
     tblgenTmpOperands = tblgenTmpOperands.drop_front(*sizeAttrIt);
   }
   return tblgenTmpOperandGroups;
 )";
 
 // The logic to build a range of either operand or result values.
 //
 // {0}: The begin iterator of the actual values.
 // {1}: The call to generate the start and length of the value range.
-const char *valueRangeReturnCode = R"(
+static const char *const valueRangeReturnCode = R"(
   auto valueRange = {1};
   return {{std::next({0}, valueRange.first),
            std::next({0}, valueRange.first + valueRange.second)};
 )";
 
-const char *typeVerifierSignature =
+static const char *const typeVerifierSignature =
     "static ::mlir::LogicalResult {0}(::mlir::Operation *op, ::mlir::Type "
     "type, ::llvm::StringRef valueKind, unsigned valueGroupStartIndex)";
 
-const char *typeVerifierErrorHandler =
+static const char *const typeVerifierErrorHandler =
     " op->emitOpError(valueKind) << \" #\" << valueGroupStartIndex << \" must "
     "be {0}, but got \" << type";
 
 static const char *const opCommentHeader = R"(
 //===----------------------------------------------------------------------===//
 // {0} {1}
 //===----------------------------------------------------------------------===//
 
static std::string replaceAllSubstrs(std::string str, const std::string &match,
   }
   return str;
 }
 
 // Returns whether the record has a value of the given name that can be returned
 // via getValueAsString.
 static inline bool hasStringAttribute(const Record &record,
                                       StringRef fieldName) {
-  auto valueInit = record.getValueInit(fieldName);
+  auto *valueInit = record.getValueInit(fieldName);
   return isa<StringInit>(valueInit);
 }
 
 static std::string getArgumentName(const Operator &op, int index) {
   const auto &operand = op.getOperand(index);
   if (!operand.name.empty())
     return std::string(operand.name);
   return std::string(formatv("{0}_{1}", generatedArgName, index));
 }
 
 // Returns true if we can use unwrapped value for the given `attr` in builders.
 static bool canUseUnwrappedRawValue(const tblgen::Attribute &attr) {
   return attr.getReturnType() != attr.getStorageType() &&
          // We need to wrap the raw value into an attribute in the builder impl
          // so we need to make sure that the attribute specifies how to do that.
          !attr.getConstBuilderTemplate().empty();
 }
 
+namespace {
+/// Helper class to select between OpAdaptor and Op code templates.
+class OpOrAdaptorHelper {
+public:
+  OpOrAdaptorHelper(const Operator &op, bool emitForOp)
+      : op(op), emitForOp(emitForOp) {}
+
+  /// Object that wraps a functor in a stream operator for interop with
+  /// llvm::formatv.
+  class Formatter {
+  public:
+    template <typename Functor>
+    Formatter(Functor &&func) : func(std::forward<Functor>(func)) {}
+
+    std::string str() const {
+      std::string result;
+      llvm::raw_string_ostream os(result);
+      os << *this;
+      return os.str();
+    }
+
+  private:
+    std::function<raw_ostream &(raw_ostream &)> func;
+
+    friend raw_ostream &operator<<(raw_ostream &os, const Formatter &fmt) {
+      return fmt.func(os);
+    }
+  };
+
+  // Generate code for getting an attribute.
+  Formatter getAttr(StringRef attrName) const {
+    return [this, attrName](raw_ostream &os) -> raw_ostream & {
+      if (!emitForOp)
+        return os << formatv("odsAttrs.get(\"{0}\")", attrName);
+      return os << formatv(opGetAttr, op.getGetterName(attrName));
+    };
+  }
+
+  // Get the prefix code for emitting an error.
+  Formatter emitErrorPrefix() const {
+    return [this](raw_ostream &os) -> raw_ostream & {
+      if (emitForOp)
+        return os << "emitOpError(";
+      return os << formatv("emitError(loc, \"'{0}' op \"",
+                           op.getOperationName());
+    };
+  }
+
+  // Get the call to get an operand or segment of operands.
+  Formatter getOperand(unsigned index) const {
+    return [this, index](raw_ostream &os) -> raw_ostream & {
+      return os << formatv(op.getOperand(index).isVariadic()
+                               ? "this->getODSOperands({0})"
+                               : "(*this->getODSOperands({0}).begin())",
+                           index);
+    };
+  }
+
+  // Get the call to get a result of segment of results.
+  Formatter getResult(unsigned index) const {
+    return [this, index](raw_ostream &os) -> raw_ostream & {
+      if (!emitForOp)
+        return os << "<no results should be generated>";
+      return os << formatv(op.getResult(index).isVariadic()
+                               ? "this->getODSResults({0})"
+                               : "(*this->getODSResults({0}).begin())",
+                           index);
+    };
+  }
+
+  // Return whether an op instance is available.
+  bool isEmittingForOp() const { return emitForOp; }
+
+  // Return the ODS operation wrapper.
+  const Operator &getOp() const { return op; }
+
+private:
+  // The operation ODS wrapper.
+  const Operator &op;
+  // True if code is being generate for an op. False for an adaptor.
+  const bool emitForOp;
+};
+} // end anonymous namespace
+
 //===----------------------------------------------------------------------===//
 // Op emitter
 //===----------------------------------------------------------------------===//
 
 namespace {
 // Helper class to emit a record into the given output stream.
 class OpEmitter {
 public:
private:
   // The emitter containing all of the locally emitted verification functions.
   const StaticVerifierFunctionEmitter &staticVerifierEmitter;
 };
 
 } // end anonymous namespace
 
 // Populate the format context `ctx` with substitutions of attributes, operands
 // and results.
-// - attrGet corresponds to the name of the function to call to get value of
-//   attribute (the generated function call returns an Attribute);
-// - operandGet corresponds to the name of the function with which to retrieve
-//   an operand (the generated function call returns an OperandRange);
-// - resultGet corresponds to the name of the function to get an result (the
-//   generated function call returns a ValueRange);
-// - opRequired whether an op instance is needed
-static void populateSubstitutions(const Operator &op, const char *attrGet,
-                                  const char *operandGet, const char *resultGet,
-                                  FmtContext &ctx, bool opRequired) {
-  // Populate substitutions for attributes and named operands.
-  for (const auto &namedAttr : op.getAttributes()) {
-    ctx.addSubst(namedAttr.name,
-                 formatv(attrGet, opRequired ? op.getGetterName(namedAttr.name)
-                                             : namedAttr.name));
-  }
+static void populateSubstitutions(const OpOrAdaptorHelper &emitHelper,
+                                  FmtContext &ctx) {
+  // Populate substitutions for attributes.
+  auto &op = emitHelper.getOp();
+  for (const auto &namedAttr : op.getAttributes())
+    ctx.addSubst(namedAttr.name, emitHelper.getAttr(namedAttr.name).str());
+
+  // Populate substitutions for named operands.
   for (int i = 0, e = op.getNumOperands(); i < e; ++i) {
     auto &value = op.getOperand(i);
-    if (value.name.empty())
-      continue;
-
-    if (value.isVariadic())
-      ctx.addSubst(value.name, formatv("{0}({1})", operandGet, i));
-    else
-      ctx.addSubst(value.name, formatv("(*{0}({1}).begin())", operandGet, i));
+    if (!value.name.empty())
+      ctx.addSubst(value.name, emitHelper.getOperand(i).str());
   }
 
   // Populate substitutions for results.
   for (int i = 0, e = op.getNumResults(); i < e; ++i) {
     auto &value = op.getResult(i);
-    if (value.name.empty())
-      continue;
-
-    if (value.isVariadic())
-      ctx.addSubst(value.name, formatv("{0}({1})", resultGet, i));
-    else
-      ctx.addSubst(value.name, formatv("(*{0}({1}).begin())", resultGet, i));
+    if (!value.name.empty())
+      ctx.addSubst(value.name, emitHelper.getResult(i).str());
   }
 }
 
-// Generate attribute verification. If emitVerificationRequiringOp is set then
-// only verification for attributes whose value depend on op being known are
-// emitted, else only verification that doesn't depend on the op being known are
-// generated.
-// - emitErrorPrefix is the prefix for the error emitting call which consists
-//   of the entire function call up to start of error message fragment;
-// - emitVerificationRequiringOp specifies whether verification should be
-//   emitted for verification that require the op to exist;
-static void genAttributeVerifier(const Operator &op, const char *attrGet,
-                                 const Twine &emitErrorPrefix,
-                                 bool emitVerificationRequiringOp,
+// Generate attribute verification. If an op instance is not available, then
+// attribute checks that require one will not be emitted.
+static void genAttributeVerifier(const OpOrAdaptorHelper &emitHelper,
                                  FmtContext &ctx, OpMethodBody &body) {
   // Check that a required attribute exists.
   //
   // {0}: Attribute variable name.
   // {1}: Emit error prefix.
   // {2}: Attribute name.
   const char *const checkRequiredAttr = R"(
     if (!{0})
       return {1}"requires attribute '{2}'");
   )";
   // Check the condition on an attribute if it is required. This assumes that
   // default values are valid.
   // TODO: verify the default value is valid (perhaps in debug mode only).
   //
   // {0}: Attribute variable name.
   // {1}: Attribute condition code.
   // {2}: Emit error prefix.
   // {3}: Attribute/constraint description.
   const char *const checkAttrCondition = R"(
     if ({0} && !({1}))
       return {2}"attribute '{3}' failed to satisfy constraint: {4}");
   )";
 
-  for (const auto &namedAttr : op.getAttributes()) {
+  for (const auto &namedAttr : emitHelper.getOp().getAttributes()) {
     const auto &attr = namedAttr.attr;
     StringRef attrName = namedAttr.name;
     if (attr.isDerivedAttr())
       continue;
 
     bool allowMissingAttr = attr.hasDefaultValue() || attr.isOptional();
     auto attrPred = attr.getPredicate();
     std::string condition = attrPred.isNull() ? "" : attrPred.getCondition();
     // If the attribute's condition needs an op but none is available, then the
     // condition cannot be emitted.
     bool canEmitCondition =
-        !StringRef(condition).contains("$_op") || emitVerificationRequiringOp;
+        !StringRef(condition).contains("$_op") || emitHelper.isEmittingForOp();
 
     // Prefix with `tblgen_` to avoid hiding the attribute accessor.
     Twine varName = tblgenNamePrefix + attrName;
 
     // If the attribute is not required and we cannot emit the condition, then
     // there is nothing to be done.
     if (allowMissingAttr && !canEmitCondition)
       continue;
 
     body << formatv("  {\n    auto {0} = {1};", varName,
-                    formatv(attrGet, emitVerificationRequiringOp
-                                         ? op.getGetterName(attrName)
-                                         : attrName));
+                    emitHelper.getAttr(attrName));
 
-    if (!allowMissingAttr)
-      body << formatv(checkRequiredAttr, varName, emitErrorPrefix, attrName);
+    if (!allowMissingAttr) {
+      body << formatv(checkRequiredAttr, varName, emitHelper.emitErrorPrefix(),
+                      attrName);
+    }
     if (canEmitCondition) {
       body << formatv(checkAttrCondition, varName,
-                      tgfmt(condition, &ctx.withSelf(varName)), emitErrorPrefix,
-                      attrName, escapeString(attr.getSummary()));
+                      tgfmt(condition, &ctx.withSelf(varName)),
+                      emitHelper.emitErrorPrefix(), attrName,
+                      escapeString(attr.getSummary()));
     }
     body << "}\n";
   }
 }
 
 OpEmitter::OpEmitter(const Operator &op,
                      const StaticVerifierFunctionEmitter &staticVerifierEmitter)
     : def(op.getDef()), op(op),
void OpEmitter::genAttrGetters() {
   // Generate named accessor with Attribute return 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.addMethodAndPrune(attr.getStorageType(), (name + "Attr").str());
     if (!method)
       return;
-    auto &body = method->body();
-    body << "  return (*this)->getAttr(" << name << "AttrName()).template ";
-    if (attr.isOptional() || attr.hasDefaultValue())
-      body << "dyn_cast_or_null<";
-    else
-      body << "cast<";
-    body << attr.getStorageType() << ">();";
+    method->body() << formatv(
+        "  return {0}.{1}<{2}>();", formatv(opGetAttr, name),
+        attr.isOptional() || attr.hasDefaultValue() ? "dyn_cast_or_null"
+                                                    : "cast",
+        attr.getStorageType());
   };
 
   for (const NamedAttribute &namedAttr : op.getAttributes()) {
     for (StringRef name : op.getGetterNames(namedAttr.name)) {
       if (namedAttr.attr.isDerivedAttr()) {
         emitDerivedAttr(name, namedAttr.attr);
       } else {
         emitAttrWithStorageType(name, namedAttr.attr);
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 setterName, StringRef getterName,
                                      Attribute attr) {
     auto *method = opClass.addMethodAndPrune(
         "void", (setterName + "Attr").str(), attr.getStorageType(), "attr");
     if (method)
-      method->body() << "  (*this)->setAttr(" << getterName
-                     << "AttrName(), attr);";
+      method->body() << formatv("  (*this)->setAttr({0}AttrName(), attr);",
+                                getterName);
   };
 
   for (const NamedAttribute &namedAttr : op.getAttributes()) {
     if (!namedAttr.attr.isDerivedAttr())
       for (auto names : llvm::zip(op.getSetterNames(namedAttr.name),
                                   op.getGetterNames(namedAttr.name)))
         emitAttrWithStorageType(std::get<0>(names), std::get<1>(names),
                                 namedAttr.attr);
   }
 }
 
 void OpEmitter::genOptionalAttrRemovers() {
   // Generate methods for removing optional attributes, instead of having to
   // use the string interface. Enables better compile time verification.
   auto emitRemoveAttr = [&](StringRef name) {
     auto upperInitial = name.take_front().upper();
     auto suffix = name.drop_front();
     auto *method = opClass.addMethodAndPrune(
         "::mlir::Attribute", ("remove" + upperInitial + suffix + "Attr").str());
     if (!method)
       return;
-    method->body() << "  return (*this)->removeAttr(" << op.getGetterName(name)
-                   << "AttrName());";
+    method->body() << formatv("  return (*this)->removeAttr({0}AttrName());",
+                              op.getGetterName(name));
   };
 
   for (const NamedAttribute &namedAttr : op.getAttributes())
     if (namedAttr.attr.isOptional())
       emitRemoveAttr(namedAttr.name);
 }
 
 // Generates the code to compute the start and end index of an operand or result
static void generateNamedOperandGetters(const Operator &op, Class &opClass,
   }
 }
 
 void OpEmitter::genNamedOperandGetters() {
   // Build the code snippet used for initializing the operand_segment_size)s
   // array.
   std::string attrSizeInitCode;
   if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) {
-    std::string attr = op.getGetterName("operand_segment_sizes") + "AttrName()";
+    std::string attr = op.getGetterName("operand_segment_sizes");
     attrSizeInitCode = formatv(opSegmentSizeAttrInitCode, attr).str();
   }
 
   generateNamedOperandGetters(
       op, opClass,
       /*isAdaptor=*/false,
       /*sizeAttrInit=*/attrSizeInitCode,
       /*rangeType=*/"::mlir::Operation::operand_range",
if (attrSizedResults && sameVariadicSize) {
     PrintFatalError(op.getLoc(),
                     "op cannot have both 'AttrSizedResultSegments' and "
                     "'SameVariadicResultSize' traits");
   }
 
   // Build the initializer string for the result segment size attribute.
   std::string attrSizeInitCode;
   if (attrSizedResults) {
-    std::string attr = op.getGetterName("result_segment_sizes") + "AttrName()";
+    std::string attr = op.getGetterName("result_segment_sizes");
     attrSizeInitCode = formatv(opSegmentSizeAttrInitCode, attr).str();
   }
 
   generateValueRangeStartAndEnd(
       opClass, "getODSResultIndexAndLength", numVariadicResults,
       numNormalResults, "getOperation()->getNumResults()", attrSizedResults,
       attrSizeInitCode, op.getResults());
 
void OpEmitter::genTypeInterfaceMethods() {
   auto &body = method->body();
   body << "  inferredReturnTypes.resize(" << op.getNumResults() << ");\n";
 
   FmtContext fctx;
   fctx.withBuilder("odsBuilder");
   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 body << "operands[" << arg.operandOrAttributeIndex()
-                    << "].getType()";
-      return body << "attributes[" << arg.operandOrAttributeIndex()
-                  << "].getType()";
-    } else {
-      return body << tgfmt(*type.getType().getBuilderCall(), &fctx);
-    }
+    if (!type.isArg())
+      return body << tgfmt(*type.getType().getBuilderCall(), &fctx);
   };
 
   for (int i = 0, e = op.getNumResults(); i != e; ++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.
void OpEmitter::genParser() {
   auto parser = def.getValueAsString("parser").ltrim().rtrim(" \t\v\f\r");
   method->body() << "  " << tgfmt(parser, &fctx);
 }
 
 void OpEmitter::genPrinter() {
   if (hasStringAttribute(def, "assemblyFormat"))
     return;
 
-  auto valueInit = def.getValueInit("printer");
+  auto *valueInit = def.getValueInit("printer");
   StringInit *stringInit = dyn_cast<StringInit>(valueInit);
   if (!stringInit)
     return;
 
   auto *method =
       opClass.addMethodAndPrune("void", "print", "::mlir::OpAsmPrinter &", "p");
   ERROR_IF_PRUNED(method, "print", op);
   FmtContext fctx;
   fctx.addSubst("cppClass", opClass.getClassName());
   auto printer = stringInit->getValue().ltrim().rtrim(" \t\v\f\r");
   method->body() << "  " << tgfmt(printer, &fctx);
 }
 
 /// Generate verification on native traits requiring attributes.
-static void genNativeTraitAttrVerifier(OpMethodBody &body, const Operator &op,
-                                       const char *const attrGet,
-                                       const Twine &emitError,
-                                       bool opRequired) {
+static void genNativeTraitAttrVerifier(OpMethodBody &body,
+                                       const OpOrAdaptorHelper &emitHelper) {
   // Check that the variadic segment sizes attribute exists and contains the
   // expected number of elements.
   //
   // {0}: Attribute name.
   // {1}: Expected number of elements.
   // {2}: "operand" or "result".
   // {3}: Attribute getter call.
   // {4}: Emit error prefix.
   const char *const checkAttrSizedValueSegmentsCode = R"(
   {
     auto sizeAttr = {3}.dyn_cast<::mlir::DenseIntElementsAttr>();
     if (!sizeAttr)
       return {4}"missing segment sizes attribute '{0}'");
     auto numElements =
         sizeAttr.getType().cast<::mlir::ShapedType>().getNumElements();
     if (numElements != {1})
       return {4}"'{0}' attribute for specifying {2} segments must have {1} "
                 "elements, but got ") << numElements;
   }
   )";
 
   // Verify a few traits first so that we can use getODSOperands() and
   // getODSResults() in the rest of the verifier.
+  auto &op = emitHelper.getOp();
   for (auto &trait : op.getTraits()) {
     auto *t = dyn_cast<tblgen::NativeTrait>(&trait);
     if (!t)
       continue;
     std::string traitName = t->getFullyQualifiedTraitName();
     if (traitName == "::mlir::OpTrait::AttrSizedOperandSegments") {
       StringRef attrName = "operand_segment_sizes";
-      body << formatv(
-          checkAttrSizedValueSegmentsCode, attrName, op.getNumOperands(),
-          "operand",
-          formatv(attrGet, opRequired ? op.getGetterName(attrName) : attrName),
-          emitError);
+      body << formatv(checkAttrSizedValueSegmentsCode, attrName,
+                      op.getNumOperands(), "operand",
+                      emitHelper.getAttr(attrName),
+                      emitHelper.emitErrorPrefix());
     } else if (traitName == "::mlir::OpTrait::AttrSizedResultSegments") {
       StringRef attrName = "result_segment_sizes";
       body << formatv(
           checkAttrSizedValueSegmentsCode, attrName, op.getNumResults(),
-          "result",
-          formatv(attrGet, opRequired ? op.getGetterName(attrName) : attrName),
-          emitError);
+          "result", emitHelper.getAttr(attrName), emitHelper.emitErrorPrefix());
     }
   }
 }
 
 void OpEmitter::genVerifier() {
   auto *method = opClass.addMethodAndPrune("::mlir::LogicalResult", "verify");
   ERROR_IF_PRUNED(method, "verify", op);
   auto &body = method->body();
 
-  genNativeTraitAttrVerifier(body, op, opGetAttr, "emitOpError(", true);
+  OpOrAdaptorHelper emitHelper(op, /*isOp=*/true);
+  genNativeTraitAttrVerifier(body, emitHelper);
 
   auto *valueInit = def.getValueInit("verifier");
   StringInit *stringInit = dyn_cast<StringInit>(valueInit);
   bool hasCustomVerify = stringInit && !stringInit->getValue().empty();
-  populateSubstitutions(op, opGetAttr, "this->getODSOperands",
-                        "this->getODSResults", verifyCtx, /*opRequired=*/true);
+  populateSubstitutions(emitHelper, verifyCtx);
 
-  genAttributeVerifier(op, opGetAttr, "emitOpError(",
-                       /*emitVerificationRequiringOp=*/true, verifyCtx, body);
+  genAttributeVerifier(emitHelper, verifyCtx, body);
   genOperandResultVerifier(body, op.getOperands(), "operand");
   genOperandResultVerifier(body, op.getResults(), "result");
 
   for (auto &trait : op.getTraits()) {
     if (auto *t = dyn_cast<tblgen::PredTrait>(&trait)) {
       body << tgfmt("  if (!($0))\n    "
                     "return emitOpError(\"failed to verify that $1\");\n",
                     &verifyCtx, tgfmt(t->getPredTemplate(), &verifyCtx),
default:
       opClass.addTrait("::mlir::OpTrait::NOperands<" + Twine(numOperands) +
                        ">::Impl");
       break;
     }
   }
 
   // Add the native and interface traits.
   for (const auto &trait : op.getTraits()) {
-    if (auto opTrait = dyn_cast<tblgen::NativeTrait>(&trait))
+    if (auto *opTrait = dyn_cast<tblgen::NativeTrait>(&trait))
       opClass.addTrait(opTrait->getFullyQualifiedTraitName());
-    else if (auto opTrait = dyn_cast<tblgen::InterfaceTrait>(&trait))
+    else if (auto *opTrait = dyn_cast<tblgen::InterfaceTrait>(&trait))
       opClass.addTrait(opTrait->getFullyQualifiedTraitName());
   }
 }
 
 void OpEmitter::genOpNameGetter() {
   auto *method = opClass.addMethodAndPrune(
       "::llvm::StringLiteral", "getOperationName",
       OpMethod::Property(OpMethod::MP_Static | OpMethod::MP_Constexpr));
private:
 
   // Add verification function. This generates a verify method for the adaptor
   // which verifies all the op-independent attribute constraints.
   void addVerification();
 
   const Operator &op;
   Class adaptor;
 };
-} // end namespace
+} // end anonymous namespace
 
 OpOperandAdaptorEmitter::OpOperandAdaptorEmitter(const Operator &op)
     : op(op), adaptor(op.getAdaptorName()) {
   adaptor.newField("::mlir::ValueRange", "odsOperands");
   adaptor.newField("::mlir::DictionaryAttr", "odsAttrs");
   adaptor.newField("::mlir::RegionRange", "odsRegions");
   const auto *attrSizedOperands =
       op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments");

 }
 
 void OpOperandAdaptorEmitter::addVerification() {
   auto *method = adaptor.addMethodAndPrune("::mlir::LogicalResult", "verify",
                                            "::mlir::Location", "loc");
   ERROR_IF_PRUNED(method, "verify", op);
   auto &body = method->body();
 
-  std::string emitError =
-      "emitError(loc, \"'" + op.getOperationName() + "' op \"";
-  genNativeTraitAttrVerifier(body, op, adaptorGetAttr, emitError,
-                             /*opRequired=*/false);
-
+  OpOrAdaptorHelper emitHelper(op, /*isOp=*/false);
+  genNativeTraitAttrVerifier(body, emitHelper);
   FmtContext verifyCtx;
-  populateSubstitutions(op, adaptorGetAttr, "getODSOperands",
-                        "<no results should be generated>", verifyCtx,
-                        /*opRequired=*/false);
-  genAttributeVerifier(op, adaptorGetAttr, emitError,
-                       /*emitVerificationRequiringOp*/ false, verifyCtx, body);
+  populateSubstitutions(emitHelper, verifyCtx);
+
+  genAttributeVerifier(emitHelper, verifyCtx, body);
 
   body << "  return ::mlir::success();";
 }
 
 void OpOperandAdaptorEmitter::emitDecl(const Operator &op, raw_ostream &os) {
   OpOperandAdaptorEmitter(op).adaptor.writeDeclTo(os);
 }