[mlir][DeclarativeParser] Emit an error if a `:` follows an attribute with a non-constant type.

[rriddle]

Summary

The attribute grammar includes an optional trailing colon type, so for attributes without a constant buildable type this will generally lead to unexpected and undesired behavior. Given that, it's better to just error out on these cases.

[lattner]

Awesome, thank you River! Is this also introducing TypedStrAttr as a new way to specify an attribute that cannot have a ": type" production in the parser? Is there a way to spell that?

[bondhugula]

mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td

23

Shouldn't this be I64? The affine map's results / inputs are Index type, but when the affine map is a constant, that constant is an int64 (and not of index bit width).

[rriddle]

In D77293#1957405, @lattner wrote:

Awesome, thank you River! Is this also introducing TypedStrAttr as a new way to specify an attribute that cannot have a ": type" production in the parser? Is there a way to spell that?

Not exactly, TypedStrAttr is intended to be a way to specify a StrAttr with a non-None type; given that NoneType is the overwhelmingly common case.

You can already use the valueType field, used here, to specify that an attribute has a static type and to print/parser without it. For example, if you have an I32Attr today it won't print/parse
the type because we can statically construct an I32 type from the builder. This revision uses that knowledge to check if the attribute parses the type or not.

[rriddle]

mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td

23

I'm basing this off of the type of the MLIR attribute, which is always Index.

https://github.com/llvm/llvm-project/blob/6acd3003755db3944f7fcbea7542bd574a41c0a0/mlir/lib/IR/AttributeDetail.h#L34

[bondhugula]

mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td

23

I see - looks like I completed missed what valueType here meant.

[rriddle]

Rebase

mlir/include/mlir/Dialect/Affine/IR/AffineOpsBase.td

 
 include "mlir/IR/OpBase.td"
 
 // Attributes containing affine maps.
 def AffineMapAttr : Attr<
     CPred<"$_self.isa<AffineMapAttr>()">, "AffineMap attribute"> {
   let storageType = [{ AffineMapAttr }];
   let returnType = [{ AffineMap }];
+  let valueType = Index;
   let constBuilderCall = "AffineMapAttr::get($0)";
 }
 
 def AffineMapArrayAttr : TypedArrayAttrBase<AffineMapAttr,
                                       "AffineMap array attribute"> {
   let constBuilderCall = "$_builder.getAffineMapArrayAttr($0)";
 }
 
 #endif // AFFINE_OPS_BASE

mlir/include/mlir/IR/OpBase.td

class BuildableType<code builder> {
   // The builder call to invoke (if specified) to construct the BuildableType.
   code builderCall = builder;
 }
 
 // Any type at all.
 def AnyType : Type<CPred<"true">, "any type">;
 
 // None type
-def NoneType : Type<CPred<"$_self.isa<NoneType>()">, "none type">;
+def NoneType : Type<CPred<"$_self.isa<NoneType>()">, "none type">,
+      BuildableType<"$_builder.getType<NoneType>()">;
 
 // Any type from the given list
 class AnyTypeOf<list<Type> allowedTypes, string description = ""> : Type<
     // Satisfy any of the allowed type's condition
     Or<!foreach(allowedtype, allowedTypes, allowedtype.predicate)>,
     !if(!eq(description, ""),
         StrJoin<!foreach(t, allowedTypes, t.description), " or ">.result,
         description)>;
def AnyAttr : Attr<CPred<"true">, "any attribute"> {
   let returnType = "Attribute";
   let convertFromStorage = "$_self";
   let constBuilderCall = "$0";
 }
 
 def BoolAttr : Attr<CPred<"$_self.isa<BoolAttr>()">, "bool attribute"> {
   let storageType = [{ BoolAttr }];
   let returnType = [{ bool }];
+  let valueType = I1;
   let constBuilderCall = "$_builder.getBoolAttr($0)";
 }
 
 // Base class for any integer (regardless of signedness semantics) attributes
 // of fixed width.
 class AnyIntegerAttrBase<AnyI attrValType, string descr> :
     TypedAttrBase<
       attrValType, "IntegerAttr",

 def F32Attr : FloatAttrBase<F32, "32-bit float attribute">;
 def F64Attr : FloatAttrBase<F64, "64-bit float attribute">;
 
 // An attribute backed by a string type.
 class StringBasedAttr<Pred condition, string descr> : Attr<condition, descr> {
   let constBuilderCall = "$_builder.getStringAttr(\"$0\")";
   let storageType = [{ StringAttr }];
   let returnType = [{ StringRef }];
+  let valueType = NoneType;
 }
 
 def StrAttr : StringBasedAttr<CPred<"$_self.isa<StringAttr>()">,
                               "string attribute">;
 
+// String attribute that has a specific value type.
+class TypedStrAttr<Type ty> : StringBasedAttr<CPred<"$_self.isa<StringAttr>()">,
+                                                    "string attribute"> {
+  let valueType = ty;
+}
+
 // Base class for attributes containing types. Example:
 //   def IntTypeAttr : TypeAttrBase<"IntegerType", "integer type attribute">
 // defines a type attribute containing an integer type.
 class TypeAttrBase<string retType, string description> :
     Attr<And<[
       CPred<"$_self.isa<TypeAttr>()">,
       CPred<"$_self.cast<TypeAttr>().getValue().isa<" # retType # ">()">]>,
     description> {
   let storageType = [{ TypeAttr }];
   let returnType = retType;
+  let valueType = NoneType;
   let convertFromStorage = "$_self.getValue().cast<" # retType # ">()";
 }
 
 def TypeAttr : TypeAttrBase<"Type", "any type attribute">;
 
 // The mere presence of unit attributes has a meaning.  Therefore, unit
 // attributes are always treated as optional and accessors to them return
 // "true" if the attribute is present and "false" otherwise.
 def UnitAttr : Attr<CPred<"$_self.isa<UnitAttr>()">, "unit attribute"> {
   let storageType = [{ UnitAttr }];
   let constBuilderCall = "$_builder.getUnitAttr()";
   let convertFromStorage = "$_self != nullptr";
   let returnType = "bool";
+  let valueType = NoneType;
   let isOptional = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // Enum attribute kinds
 
 // Additional information for an enum attribute case.
 class EnumAttrCaseInfo<string sym, int intVal, string strVal> {

 
 //===----------------------------------------------------------------------===//
 // Composite attribute kinds
 
 class DictionaryAttrBase : Attr<CPred<"$_self.isa<DictionaryAttr>()">,
                           "dictionary of named attribute values"> {
   let storageType = [{ DictionaryAttr }];
   let returnType = [{ DictionaryAttr }];
+  let valueType = NoneType;
   let convertFromStorage = "$_self";
 }
 
 def DictionaryAttr : DictionaryAttrBase;
 
 class ElementsAttrBase<Pred condition, string description> :
     Attr<condition, description> {
   let storageType = [{ ElementsAttr }];

 class RankedF32ElementsAttr<list<int> dims> : RankedFloatElementsAttr<32, dims>;
 class RankedF64ElementsAttr<list<int> dims> : RankedFloatElementsAttr<64, dims>;
 
 // Base class for array attributes.
 class ArrayAttrBase<Pred condition, string description> :
     Attr<condition, description> {
   let storageType = [{ ArrayAttr }];
   let returnType = [{ ArrayAttr }];
+  let valueType = NoneType;
   let convertFromStorage = "$_self";
 }
 
 def ArrayAttr : ArrayAttrBase<CPred<"$_self.isa<ArrayAttr>()">,
                               "array attribute">;
 
 // Base class for array attributes whose elements are of the same kind.
 // `element` specifies the element attribute kind stored in this array.
class StructAttr<string name, Dialect dialect,
   list<StructFieldAttr> fields = attributes;
 }
 
 // Attributes containing symbol references.
 def SymbolRefAttr : Attr<CPred<"$_self.isa<SymbolRefAttr>()">,
                         "symbol reference attribute"> {
   let storageType = [{ SymbolRefAttr }];
   let returnType = [{ SymbolRefAttr }];
+  let valueType = NoneType;
   let constBuilderCall = "$_builder.getSymbolRefAttr($0)";
   let convertFromStorage = "$_self";
 }
 def FlatSymbolRefAttr : Attr<CPred<"$_self.isa<FlatSymbolRefAttr>()">,
                                    "flat symbol reference attribute"> {
   let storageType = [{ FlatSymbolRefAttr }];
   let returnType = [{ StringRef }];
+  let valueType = NoneType;
   let constBuilderCall = "$_builder.getSymbolRefAttr($0)";
   let convertFromStorage = "$_self.getValue()";
 }
 
 def SymbolRefArrayAttr :
   TypedArrayAttrBase<SymbolRefAttr, "symbol ref array attribute"> {
   let constBuilderCall = ?;
 }

mlir/test/IR/attribute.mlir

 
 //===----------------------------------------------------------------------===//
 // Test StringAttr with custom type
 //===----------------------------------------------------------------------===//
 
 // CHECK-LABEL: func @string_attr_custom_type
 func @string_attr_custom_type() {
   // CHECK: "string_data" : !foo.string
-  test.string_attr_with_type "string_data"
+  test.string_attr_with_type "string_data" : !foo.string
   return
 }
 
 // -----
 
 //===----------------------------------------------------------------------===//
 // Test StrEnumAttr
 //===----------------------------------------------------------------------===//

mlir/test/lib/Dialect/Test/TestOps.td

let arguments = (ins
       Confined<I64Attr, [IntPositive]>:$i64attr
   );
 }
 
 def TypeArrayAttrOp : TEST_Op<"type_array_attr"> {
   let arguments = (ins TypeArrayAttr:$attr);
 }
 def TypeStringAttrWithTypeOp : TEST_Op<"string_attr_with_type"> {
-  let arguments = (ins StrAttr:$attr);
-  let printer = [{ p << getAttr("attr"); }];
-  let parser = [{
-    Attribute attr;
-    Type stringType = OpaqueType::get(Identifier::get("foo",
-                                      result.getContext()), "string",
-                                      result.getContext());
-    return parser.parseAttribute(attr, stringType, "attr", result.attributes);
-  }];
+  let arguments = (ins TypedStrAttr<AnyType>:$attr);
+  let assemblyFormat = "$attr attr-dict";
 }
 
 def StrCaseA: StrEnumAttrCase<"A">;
 def StrCaseB: StrEnumAttrCase<"B">;
 
 def SomeStrEnum: StrEnumAttr<
   "SomeStrEnum", "", [StrCaseA, StrCaseB]>;
 

mlir/test/mlir-tblgen/op-format-spec.td

-// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I %S/../../include %s 2>&1 | FileCheck %s --dump-input-on-failure
+// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I %S/../../include %s -o=%t 2>&1 | FileCheck %s --dump-input-on-failure
 
 // This file contains tests for the specification of the declarative op format.
 
 include "mlir/IR/OpBase.td"
 
 def TestDialect : Dialect {
   let name = "test";
 }
def VariableInvalidF : TestFormat_Op<"variable_invalid_f", [{
   let successors = (successor AnySuccessor:$successor);
 }
 // CHECK: error: successor 'successor' is already bound
 def VariableInvalidG : TestFormat_Op<"variable_invalid_g", [{
   successors $successor attr-dict
 }]> {
   let successors = (successor AnySuccessor:$successor);
 }
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr` which does not have a buildable type
+def VariableInvalidH : TestFormat_Op<"variable_invalid_h", [{
+  $attr `:` attr-dict
+}]>, Arguments<(ins ElementsAttr:$attr)>;
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr` which does not have a buildable type
+def VariableInvalidI : TestFormat_Op<"variable_invalid_i", [{
+  (`foo` $attr^)? `:` attr-dict
+}]>, Arguments<(ins OptionalAttr<ElementsAttr>:$attr)>;
+// CHECK-NOT: error:
+def VariableInvalidJ : TestFormat_Op<"variable_invalid_j", [{
+  $attr `:` attr-dict
+}]>, Arguments<(ins OptionalAttr<I1Attr>:$attr)>;
+def VariableInvalidK : TestFormat_Op<"variable_invalid_k", [{
+  (`foo` $attr^)? `:` attr-dict
+}]>, Arguments<(ins OptionalAttr<I1Attr>:$attr)>;
 
 //===----------------------------------------------------------------------===//
 // Coverage Checks
 //===----------------------------------------------------------------------===//
 
 // CHECK: error: format missing instance of result #0('result') type
 def ZCoverageInvalidA : TestFormat_Op<"variable_invalid_a", [{
   attr-dict

mlir/tools/mlir-tblgen/OpFormatGen.cpp

 class VariableElement : public Element {
 public:
   VariableElement(const VarT *var) : Element(kindVal), var(var) {}
   static bool classof(const Element *element) {
     return element->getKind() == kindVal;
   }
   const VarT *getVar() { return var; }
 
-private:
+protected:
   const VarT *var;
 };
 
 /// This class represents a variable that refers to an attribute argument.
-using AttributeVariable =
-    VariableElement<NamedAttribute, Element::Kind::AttributeVariable>;
+struct AttributeVariable
+    : public VariableElement<NamedAttribute, Element::Kind::AttributeVariable> {
+  using VariableElement<NamedAttribute,
+                        Element::Kind::AttributeVariable>::VariableElement;
+
+  /// Return the constant builder call for the type of this attribute, or None
+  /// if it doesn't have one.
+  Optional<StringRef> getTypeBuilder() const {
+    Optional<Type> attrType = var->attr.getValueType();
+    return attrType ? attrType->getBuilderCall() : llvm::None;
+  }
+};
 
 /// This class represents a variable that refers to an operand argument.
 using OperandVariable =
     VariableElement<NamedTypeConstraint, Element::Kind::OperandVariable>;
 
 /// This class represents a variable that refers to a result.
 using ResultVariable =
     VariableElement<NamedTypeConstraint, Element::Kind::ResultVariable>;
if (canFormatEnumAttr(var)) {
       body << formatv(enumAttrParserCode, var->name, enumAttr.getCppNamespace(),
                       enumAttr.getStringToSymbolFnName(), attrBuilderStr);
       return;
     }
 
     // If this attribute has a buildable type, use that when parsing the
     // attribute.
     std::string attrTypeStr;
-    if (Optional<Type> attrType = var->attr.getValueType()) {
-      if (Optional<StringRef> typeBuilder = attrType->getBuilderCall()) {
-        llvm::raw_string_ostream os(attrTypeStr);
-        os << ", " << tgfmt(*typeBuilder, &attrTypeCtx);
-      }
-    }
+    if (Optional<StringRef> typeBuilder = attr->getTypeBuilder()) {
 
     body << formatv(attrParserCode, var->attr.getStorageType(), var->name,
                     attrTypeStr);
   } else if (auto *operand = dyn_cast<OperandVariable>(element)) {
     bool isVariadic = operand->getVar()->isVariadic();
     body << formatv(isVariadic ? variadicOperandParserCode : operandParserCode,
                     operand->getVar()->name);
   } else if (auto *successor = dyn_cast<SuccessorVariable>(element)) {
if (auto *attr = dyn_cast<AttributeVariable>(element)) {
     if (canFormatEnumAttr(var)) {
       const EnumAttr &enumAttr = cast<EnumAttr>(var->attr);
       body << "  p << \"\\\"\" << " << enumAttr.getSymbolToStringFnName() << "("
            << var->name << "()) << \"\\\"\";\n";
       return;
     }
 
     // Elide the attribute type if it is buildable.
-    Optional<Type> attrType = var->attr.getValueType();
-    if (attrType && attrType->getBuilderCall())
+    if (attr->getTypeBuilder())
       body << "  p.printAttributeWithoutType(" << var->name << "Attr());\n";
     else
       body << "  p.printAttribute(" << var->name << "Attr());\n";
   } else if (auto *operand = dyn_cast<OperandVariable>(element)) {
     body << "  p << " << operand->getVar()->name << "();\n";
   } else if (auto *successor = dyn_cast<SuccessorVariable>(element)) {
     const NamedSuccessor *var = successor->getVar();
     if (var->isVariadic())
private:
   /// This struct represents a type resolution instance. It includes a specific
   /// type as well as an optional transformer to apply to that type in order to
   /// properly resolve the type of a variable.
   struct TypeResolutionInstance {
     const NamedTypeConstraint *type;
     Optional<StringRef> transformer;
   };
 
+  /// Verify the state of operation attributes within the format.
+  LogicalResult verifyAttributes(llvm::SMLoc loc);
+
+  /// Verify the state of operation operands within the format.
+  LogicalResult
+  verifyOperands(llvm::SMLoc loc,
+                 llvm::StringMap<TypeResolutionInstance> &variableTyResolver);
+
+  /// Verify the state of operation results within the format.
+  LogicalResult
+  verifyResults(llvm::SMLoc loc,
+                llvm::StringMap<TypeResolutionInstance> &variableTyResolver);
+
+  /// Verify the state of operation successors within the format.
+  LogicalResult verifySuccessors(llvm::SMLoc loc);
+
   /// Given the values of an `AllTypesMatch` trait, check for inferable type
   /// resolution.
   void handleAllTypesMatchConstraint(
       ArrayRef<StringRef> values,
       llvm::StringMap<TypeResolutionInstance> &variableTyResolver);
   /// Check for inferable type resolution given all operands, and or results,
   /// have the same type. If 'includeResults' is true, the results also have the
   /// same type as all of the operands.
if (def.isSubClassOf("AllTypesMatch")) {
       handleSameTypesConstraint(variableTyResolver, /*includeResults=*/true);
     } else if (def.isSubClassOf("TypesMatchWith")) {
       if (const auto *lhsArg = findSeenArg(def.getValueAsString("lhs")))
         variableTyResolver[def.getValueAsString("rhs")] = {
             lhsArg, def.getValueAsString("transformer")};
     }
   }
 
-  // Check that all of the result types can be inferred.
-  auto &buildableTypes = fmt.buildableTypes;
-  if (!fmt.allResultTypes) {
-    for (unsigned i = 0, e = op.getNumResults(); i != e; ++i) {
-      if (seenResultTypes.test(i))
+  // Verify the state of the various operation components.
+  if (failed(verifyAttributes(loc)) ||
+      failed(verifyResults(loc, variableTyResolver)) ||
+      failed(verifyOperands(loc, variableTyResolver)) ||
+      failed(verifySuccessors(loc)))
+    return failure();
+
+  // Check to see if we are formatting all of the operands.
+  fmt.allOperands = llvm::any_of(fmt.elements, [](auto &elt) {
+    return isa<OperandsDirective>(elt.get());
+  });
+  return success();
+}
+
+LogicalResult FormatParser::verifyAttributes(llvm::SMLoc loc) {
+  // Check that there are no `:` literals after an attribute without a constant
+  // type. The attribute grammar contains an optional trailing colon type, which
+  // can lead to unexpected and generally unintended behavior. Given that, it is
+  // better to just error out here instead.
+  using ElementsIterT = llvm::pointee_iterator<
+      std::vector<std::unique_ptr<Element>>::const_iterator>;
+  SmallVector<std::pair<ElementsIterT, ElementsIterT>, 1> iteratorStack;
+  iteratorStack.emplace_back(fmt.elements.begin(), fmt.elements.end());
+  while (!iteratorStack.empty()) {
+    auto &stackIt = iteratorStack.back();
+    ElementsIterT &it = stackIt.first, e = stackIt.second;
+    while (it != e) {
+      Element *element = &*(it++);
+
+      // Traverse into optional groups.
+      if (auto *optional = dyn_cast<OptionalElement>(element)) {
+        auto elements = optional->getElements();
+        iteratorStack.emplace_back(elements.begin(), elements.end());
+        break;
+      }
+
+      // We are checking for an attribute element followed by a `:`, so there is
+      // no need to check the end.
+      if (it == e && iteratorStack.size() == 1)
+        break;
+
+      // Check for an attribute with a constant type builder, followed by a `:`.
+      auto *prevAttr = dyn_cast<AttributeVariable>(element);
+      if (!prevAttr || prevAttr->getTypeBuilder())
         continue;
 
-      // Check to see if we can infer this type from another variable.
-      auto varResolverIt = variableTyResolver.find(op.getResultName(i));
-      if (varResolverIt != variableTyResolver.end()) {
-        fmt.resultTypes[i].setVariable(varResolverIt->second.type,
-                                       varResolverIt->second.transformer);
+      // Check the next iterator within the stack for literal elements.
+      for (auto &nextItPair : iteratorStack) {
+        ElementsIterT nextIt = nextItPair.first, nextE = nextItPair.second;
+        for (; nextIt != nextE; ++nextIt) {
+          // Skip any trailing optional groups or attribute dictionaries.
+          if (isa<AttrDictDirective>(*nextIt) || isa<OptionalElement>(*nextIt))
             continue;
-      }
 
-      // If the result is not variadic, allow for the case where the type has a
-      // builder that we can use.
-      NamedTypeConstraint &result = op.getResult(i);
-      Optional<StringRef> builder = result.constraint.getBuilderCall();
-      if (!builder || result.constraint.isVariadic()) {
-        return emitError(loc, "format missing instance of result #" + Twine(i) +
-                                  "('" + result.name + "') type");
+          // We are only interested in `:` literals.
+          auto *literal = dyn_cast<LiteralElement>(&*nextIt);
+          if (!literal || literal->getLiteral() != ":")
+            break;
+
+          // TODO: Use the location of the literal element itself.
+          return emitError(
+              loc, llvm::formatv("format ambiguity caused by `:` literal found "
+                                 "after attribute `{0}` which does not have "
+                                 "a buildable type",
+                                 prevAttr->getVar()->name));
         }
-      // Note in the format that this result uses the custom builder.
-      auto it = buildableTypes.insert({*builder, buildableTypes.size()});
-      fmt.resultTypes[i].setBuilderIdx(it.first->second);
       }
     }
+    if (it == e)
+      iteratorStack.pop_back();
+  }
+  return success();
+}
 
+LogicalResult FormatParser::verifyOperands(
+    llvm::SMLoc loc,
+    llvm::StringMap<TypeResolutionInstance> &variableTyResolver) {
   // Check that all of the operands are within the format, and their types can
   // be inferred.
+  auto &buildableTypes = fmt.buildableTypes;
   for (unsigned i = 0, e = op.getNumOperands(); i != e; ++i) {
     NamedTypeConstraint &operand = op.getOperand(i);
 
     // Check that the operand itself is in the format.
     if (!hasAllOperands && !seenOperands.count(&operand)) {
       return emitError(loc, "format missing instance of operand #" + Twine(i) +
                                 "('" + operand.name + "')");
     }
for (unsigned i = 0, e = op.getNumOperands(); i != e; ++i) {
     Optional<StringRef> builder = operand.constraint.getBuilderCall();
     if (!builder || (hasAllOperands && operand.isVariadic())) {
       return emitError(loc, "format missing instance of operand #" + Twine(i) +
                                 "('" + operand.name + "') type");
     }
     auto it = buildableTypes.insert({*builder, buildableTypes.size()});
     fmt.operandTypes[i].setBuilderIdx(it.first->second);
   }
+  return success();
+}
+
+LogicalResult FormatParser::verifyResults(
+    llvm::SMLoc loc,
+    llvm::StringMap<TypeResolutionInstance> &variableTyResolver) {
+  // If we format all of the types together, there is nothing to check.
+  if (fmt.allResultTypes)
+    return success();
+
+  // Check that all of the result types can be inferred.
+  auto &buildableTypes = fmt.buildableTypes;
+  for (unsigned i = 0, e = op.getNumResults(); i != e; ++i) {
+    if (seenResultTypes.test(i))
+      continue;
+
+    // Check to see if we can infer this type from another variable.
+    auto varResolverIt = variableTyResolver.find(op.getResultName(i));
+    if (varResolverIt != variableTyResolver.end()) {
+      fmt.resultTypes[i].setVariable(varResolverIt->second.type,
+                                     varResolverIt->second.transformer);
+      continue;
+    }
+
+    // If the result is not variadic, allow for the case where the type has a
+    // builder that we can use.
+    NamedTypeConstraint &result = op.getResult(i);
+    Optional<StringRef> builder = result.constraint.getBuilderCall();
+    if (!builder || result.constraint.isVariadic()) {
+      return emitError(loc, "format missing instance of result #" + Twine(i) +
+                                "('" + result.name + "') type");
+    }
+    // Note in the format that this result uses the custom builder.
+    auto it = buildableTypes.insert({*builder, buildableTypes.size()});
+    fmt.resultTypes[i].setBuilderIdx(it.first->second);
+  }
+  return success();
+}
 
+LogicalResult FormatParser::verifySuccessors(llvm::SMLoc loc) {
   // Check that all of the successors are within the format.
-  if (!hasAllSuccessors) {
-    for (unsigned i = 0, e = op.getNumSuccessors(); i != e; ++i) {
-      const NamedSuccessor &successor = op.getSuccessor(i);
-      if (!seenSuccessors.count(&successor)) {
-        return emitError(loc, "format missing instance of successor #" +
-                                  Twine(i) + "('" + successor.name + "')");
-      }
-    }
-  }
-
-  // Check to see if we are formatting all of the operands.
-  fmt.allOperands = llvm::any_of(fmt.elements, [](auto &elt) {
-    return isa<OperandsDirective>(elt.get());
-  });
+  if (hasAllSuccessors)
+    return success();
+
   return success();
 }
 
 void FormatParser::handleAllTypesMatchConstraint(
     ArrayRef<StringRef> values,
     llvm::StringMap<TypeResolutionInstance> &variableTyResolver) {
   for (unsigned i = 0, e = values.size(); i != e; ++i) {
     // Check to see if this value matches a resolved operand or result type.