[mlir][ods] Fix verification of attribute + colon type ambiguity

[Mogball]

Summary

An attribute without a type builder followed by a colon in an assembly format is potentially ambiguous because the parser will read ahead to parse the colon-type and pass this as the type argument to the attribute's constructor.

However, the previous verifier that checks for this ambiguity erroneously produces an error in the case of

let assemblyFormat = "( `(` $attr `)` )? `:`";

This patch fixes the bug by implementing a checker that correctly handles all edge cases, including very strange assembly formats like:

let assemblyFormat = "( `(` $attr ) : (`>`)? attr-dict (`>` $a^) : (`<`)? `:`";

[Mogball]

add extra test case

[rriddle]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

2360

Shouldn't we be checking optional elements as well?

$attr (`:` $foo^)?

Feels like we should diagnose this.

2453

Can you split this out into a separate function? This logic in this function is kind of hard to follow.

2517

Can you just early return from the other if and sink this to the bottom?

[Mogball]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

2360

Optional elements go away before calling into this function. See the comment on the function below.

2517

This just moved code. Let me drop it from this diff.

[Mogball]

simplify the implementation of the attribute-colon verifier and catch more cases

[Mogball]

clang-format

[rriddle]

Looks good. I think we can do some more generalizations of the verification though (doesn't have to be in this commit). There are various other ambiguities that it'd be nice if they were simple to add the core checks for (e.g. attr-dict next to a region).

[Mogball]

Oh yeah, I forgot about that one (and I think I filed that bug...).

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

 def FormatCustomDirectiveAttrDict
     : TEST_Op<"format_custom_directive_attrdict"> {
   let arguments = (ins I64Attr:$attr, OptionalAttr<I64Attr>:$optAttr);
   let assemblyFormat = [{
     custom<CustomDirectiveAttrDict>( attr-dict )
   }];
 }
 
+def FormatLiteralFollowingOptionalGroup
+    : TEST_Op<"format_literal_following_optional_group"> {
+  let arguments = (ins TypeAttr:$type, OptionalAttr<AnyAttr>:$value);
+  let assemblyFormat = "(`(` $value^ `)`)? `:` $type attr-dict";
+}
+
 //===----------------------------------------------------------------------===//
 // AllTypesMatch type inference
 
 def FormatAllTypesMatchVarOp : TEST_Op<"format_all_types_match_var", [
     AllTypesMatch<["value1", "value2", "result"]>
   ]> {
   let arguments = (ins AnyType:$value1, AnyType:$value2);
   let results = (outs AnyType:$result);

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

def VariableInvalidF : TestFormat_Op<[{
   let successors = (successor AnySuccessor:$successor);
 }
 // CHECK: error: successor 'successor' is already bound
 def VariableInvalidG : TestFormat_Op<[{
   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<[{
-  $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<[{
-  (`foo` $attr^)? `:` attr-dict
-}]>, Arguments<(ins OptionalAttr<ElementsAttr>:$attr)>;
-// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr` which does not have a buildable type
-def VariableInvalidJ : TestFormat_Op<[{
-  $attr ` ` `:` attr-dict
-}]>, Arguments<(ins ElementsAttr:$attr)>;
 // CHECK: error: region 'region' is already bound
 def VariableInvalidK : TestFormat_Op<[{
   $region $region attr-dict
 }]> {
   let regions = (region AnyRegion:$region);
 }
 // CHECK: error: region 'region' is already bound
 def VariableInvalidL : TestFormat_Op<[{

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

+// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=false -I %S/../../include %s -o=%t 2>&1 | FileCheck %s
+
+include "mlir/IR/OpBase.td"
+
+def TestDialect : Dialect {
+  let name = "test";
+}
+class TestFormat_Op<string fmt, list<Trait> traits = []>
+    : Op<TestDialect, "format_op", traits> {
+  let assemblyFormat = fmt;
+}
+
+//===----------------------------------------------------------------------===//
+// Format ambiguity caused by attribute followed by colon literal
+//===----------------------------------------------------------------------===//
+
+// Test attribute followed by a colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeA : TestFormat_Op<[{
+  $attr `:` attr-dict
+}]>, Arguments<(ins AnyAttr:$attr)>;
+
+// Test optional attribute followed by colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeB : TestFormat_Op<[{
+  (`foo` $attr^)? `:` attr-dict
+}]>, Arguments<(ins OptionalAttr<AnyAttr>:$attr)>;
+
+// Test attribute followed by whitespace and then colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeC : TestFormat_Op<[{
+  $attr ` ` `:` attr-dict
+}]>, Arguments<(ins AnyAttr:$attr)>;
+
+// Test attribute followed by optional dictionary and then colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeD : TestFormat_Op<[{
+  $attr attr-dict `:`
+}]>, Arguments<(ins AnyAttr:$attr)>;
+
+// Test attribute followed by optional group and then colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeE : TestFormat_Op<[{
+  $attr ($a^)? `:` attr-dict type($a)
+}]>, Arguments<(ins AnyAttr:$attr, Optional<I32>:$a)>;
+
+// Test attribute followed by optional group with literals and then colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeF : TestFormat_Op<[{
+  $attr (`(` $a^ `)`)? `:` attr-dict (`(` type($a)^ `)`)?
+}]>, Arguments<(ins AnyAttr:$attr, Optional<I32>:$a)>;
+
+// Test attribute followed by optional group with else group.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeG : TestFormat_Op<[{
+  $attr (`(` $a^ `)`) : (`foo`)? `:` attr-dict (`(` type($a)^ `)`)?
+}]>, Arguments<(ins AnyAttr:$attr, Optional<I32>:$a)>;
+
+// Test attribute followed by optional group with colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeH : TestFormat_Op<[{
+  $attr (` ` `:` $a^ `)`)? attr-dict (`(` type($a)^ `)`)?
+}]>, Arguments<(ins AnyAttr:$attr, Optional<I32>:$a)>;
+
+// Test attribute followed by optional group with colon in else group.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeI : TestFormat_Op<[{
+  $attr (`(` $a^ `)`) : (`:`)? attr-dict (`(` type($a)^ `)`)?
+}]>, Arguments<(ins AnyAttr:$attr, Optional<I32>:$a)>;
+
+// Test attribute followed by two optional groups and then a colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeJ : TestFormat_Op<[{
+  $attr (`(` $a^ type($a) `)`) : (`foo`)? ` ` attr-dict (`(` $b^ type($b) `)`)?
+  `:`
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins AnyAttr:$attr, Optional<I32>:$a, Optional<I32>:$b)>;
+
+// Test attribute followed by two optional groups and then a colon in the else
+// group.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeK : TestFormat_Op<[{
+  $attr (`(` $a^ type($a) `)`) : (`foo`)? ` ` attr-dict
+  (`(` $b^ type($b) `)`) : (`:`)?
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins AnyAttr:$attr, Optional<I32>:$a, Optional<I32>:$b)>;
+
+// Test attribute followed by two optional groups with guarded colons but then a
+// colon.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeL : TestFormat_Op<[{
+  $attr (`(` $a^ `:` type($a) `)`) : (`foo` `:`)? ` ` attr-dict
+  (`(` $b^ `:` type($b) `)`) : (`foo` `:`)? `:`
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins AnyAttr:$attr, Optional<I32>:$a, Optional<I32>:$b)>;
+
+// Test optional attribute followed by optional groups with a colon along one
+// path.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeM : TestFormat_Op<[{
+  (`(` $attr^ ` `)? (`(` $a^ `:` type($a) `)`) : (`foo` `:`)? ` ` attr-dict
+  (`(` $b^ `:` type($b) `)`) : (`foo` `:`)? `:`
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins OptionalAttr<AnyAttr>:$attr, Optional<I32>:$a,
+                 Optional<I32>:$b)>;
+
+// Test optional attribute followed by optional groups with a colon along one
+// path inside an optional group.
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr`
+def AmbiguousTypeN : TestFormat_Op<[{
+  (`(` $attr^ ` `)? (`(` $a^ `:` type($a) `)`) : (`foo` `:`)? ` ` attr-dict
+  (`(` $b^ `:` type($b) `)`) : (`:`)?
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins OptionalAttr<AnyAttr>:$attr, Optional<I32>:$a,
+                 Optional<I32>:$b)>;
+
+// CHECK-NOT: error
+
+// Test attribute followed by two optional groups with guarded colons.
+def ValidTypeA : TestFormat_Op<[{
+  $attr (`(` $a^ `:` type($a) `)`) : (`foo` `:`)? ` ` attr-dict
+  (`(` $b^ `:` type($b) `)`) : (`foo` `:`)? ` ` `(` `:`
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins AnyAttr:$attr, Optional<I32>:$a, Optional<I32>:$b)>;
+
+// Test optional attribute followed by two optional groups with guarded colons.
+def ValidTypeB : TestFormat_Op<[{
+  (`(` $attr^ ` `)? (`(` $a^ `:` type($a) `)`) : (`foo` `:`)? ` ` attr-dict
+  (`(` $b^ `:` type($b) `)`) : (`foo` `:`)? ` ` `(` `:`
+}], [AttrSizedOperandSegments]>,
+  Arguments<(ins OptionalAttr<AnyAttr>:$attr, Optional<I32>:$a,
+                 Optional<I32>:$b)>;
+
+// Test optional attribute guarded colon along within segment.
+def ValidTypeC : TestFormat_Op<[{
+  (`(` $attr^ `)`) : (`:`)? attr-dict `:`
+}]>, Arguments<(ins OptionalAttr<AnyAttr>:$attr)>;

mlir/tools/mlir-tblgen/OpFormatGen.cpp

 //===- OpFormatGen.cpp - MLIR operation asm format generator --------------===//
 //
 // 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 "OpFormatGen.h"
 #include "FormatGen.h"
 #include "OpClass.h"
 #include "mlir/Support/LLVM.h"
-#include "mlir/Support/LogicalResult.h"
 #include "mlir/TableGen/Class.h"
 #include "mlir/TableGen/Format.h"
-#include "mlir/TableGen/GenInfo.h"
-#include "mlir/TableGen/Interfaces.h"
 #include "mlir/TableGen/Operator.h"
 #include "mlir/TableGen/Trait.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/Support/Signals.h"
-#include "llvm/TableGen/Error.h"
+#include "llvm/Support/SourceMgr.h"
 #include "llvm/TableGen/Record.h"
+#include <numeric>
 
 #define DEBUG_TYPE "mlir-tblgen-opformatgen"
 
 using namespace mlir;
 using namespace mlir::tblgen;
 
 //===----------------------------------------------------------------------===//
 // VariableElement
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 {
     ConstArgument resolver;
     Optional<StringRef> transformer;
   };
 
-  using ElementsItT = ArrayRef<FormatElement *>::iterator;
-
   /// Verify the state of operation attributes within the format.
   LogicalResult verifyAttributes(SMLoc loc, ArrayRef<FormatElement *> elements);
+
   /// Verify the attribute elements at the back of the given stack of iterators.
-  LogicalResult verifyAttributes(
-      SMLoc loc,
-      SmallVectorImpl<std::pair<ElementsItT, ElementsItT>> &iteratorStack);
+  LogicalResult verifyAttributeColonType(SMLoc loc,
+                                         ArrayRef<FormatElement *> elements);
+
+  /// Verify the assembly format elements to ensure there are no ambiguities
+  /// with order-independent lists.
+  LogicalResult verifyOIListElements(SMLoc loc,
+                                     ArrayRef<FormatElement *> elements);
 
   /// Verify the state of operation operands within the format.
   LogicalResult
   verifyOperands(SMLoc loc,
                  llvm::StringMap<TypeResolutionInstance> &variableTyResolver);
 
   /// Verify the state of operation regions within the format.
   LogicalResult verifyRegions(SMLoc loc);
 
   /// Verify the state of operation results within the format.
   LogicalResult
   verifyResults(SMLoc loc,
                 llvm::StringMap<TypeResolutionInstance> &variableTyResolver);
 
   /// Verify the state of operation successors within the format.
   LogicalResult verifySuccessors(SMLoc loc);
 
-  LogicalResult verifyOIListElements(SMLoc loc,
-                                     ArrayRef<FormatElement *> elements);
-
   /// 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")) {
       }
     }
   }
 
   // Verify the state of the various operation components.
   if (failed(verifyAttributes(loc, elements)) ||
       failed(verifyResults(loc, variableTyResolver)) ||
       failed(verifyOperands(loc, variableTyResolver)) ||
-      failed(verifyRegions(loc)) || failed(verifySuccessors(loc)) ||
-      failed(verifyOIListElements(loc, elements)))
+      failed(verifyRegions(loc)) || failed(verifySuccessors(loc)))
     return failure();
 
   // Collect the set of used attributes in the format.
   fmt.usedAttributes = seenAttrs.takeVector();
   return success();
 }
 
 LogicalResult
 OpFormatParser::verifyAttributes(SMLoc loc,
                                  ArrayRef<FormatElement *> elements) {
   // 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.
-  SmallVector<std::pair<ElementsItT, ElementsItT>, 1> iteratorStack;
-  iteratorStack.emplace_back(elements.begin(), elements.end());
-  while (!iteratorStack.empty())
-    if (failed(verifyAttributes(loc, iteratorStack)))
-      return ::failure();
+  if (failed(verifyAttributeColonType(loc, elements)))
+    return failure();
+
+  // Verify the format elements with respect to order-independent lists.
+  if (failed(verifyOIListElements(loc, elements)))
+    return failure();
 
   // Check for VariadicOfVariadic variables. The segment attribute of those
   // variables will be infered.
   for (const NamedTypeConstraint *var : seenOperands) {
     if (var->constraint.isVariadicOfVariadic()) {
       fmt.inferredAttributes.insert(
           var->constraint.getVariadicOfVariadicSegmentSizeAttr());
     }
   }
 
   return success();
 }
-/// Verify the attribute elements at the back of the given stack of iterators.
-LogicalResult OpFormatParser::verifyAttributes(
-    SMLoc loc,
-    SmallVectorImpl<std::pair<ElementsItT, ElementsItT>> &iteratorStack) {
-  auto &stackIt = iteratorStack.back();
-  ElementsItT &it = stackIt.first, e = stackIt.second;
-  while (it != e) {
-    FormatElement *element = *(it++);
-
-    // Traverse into optional groups.
-    if (auto *optional = dyn_cast<OptionalElement>(element)) {
-      auto thenElements = optional->getThenElements();
-      iteratorStack.emplace_back(thenElements.begin(), thenElements.end());
 
-      auto elseElements = optional->getElseElements();
-      iteratorStack.emplace_back(elseElements.begin(), elseElements.end());
+/// Given a segment of a parsing path, check whether the first non-optional
+/// element is a colon. Returns failure if colon was found, meaning the path is
+/// invalid. Returns success is the path is proved to be valid, and returns None
+/// if iteration reached the end of the path.
+static Optional<LogicalResult> checkPathSegmentForColon(
+    function_ref<LogicalResult(const Twine &)> emitError, StringRef attrName,
+    iterator_range<ArrayRef<FormatElement *>::iterator> path) {
+  for (FormatElement *element : path) {
+    // Skip whitespaces and attribute dictionaries as they are optionally
+    // parsed.
+    if (isa<WhitespaceElement, AttrDictDirective>(element))
+      continue;
+    // If we encounter anything other than `:`, this parsing path is valid.
+    auto *literal = dyn_cast<LiteralElement>(element);
+    if (!literal || literal->getSpelling() != ":")
       return success();
-    }
+    return emitError(
+        llvm::formatv("format ambiguity caused by `:` literal found after "
+                      "attribute `{0}` which does not have a buildable type",
+                      attrName));
+  }
+  return llvm::None;
 
-    // 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;
+// We need to check for an attribute followed by a colon literal, skipping any
+// optional elements, along all possible parsing paths. This function will
+// "instantiate" optional groups into a series of path segments. Consider the
+// following assembly format:
+//
+// (`foo` $attr^)? attr-dict (`(` $a^ `)`) : (`foo`)? (`:` $b^)?
+//
+// When the optional groups are instantiated, the parsing paths will look like:
+//
+// [`foo`, $attr] [attr-dict] [`(`, $a, `)`] [`:`, $b]
+// []                         [`foo`]        []
+//                            []
+//
+// This function will then find any attribute elements among the path segments
+// and check until the end of that path segment for a colon. If it reaches the
+// end, it will check the next group of segments for a possible ambiguity. If
+// one is found, the function returns failure. If it reaches the end of any
+// segment, it repeats on the next group.
+//
+// TODO: Make this less complicated.
+LogicalResult OpFormatParser::verifyAttributeColonType(
+    SMLoc loc,
+    ArrayRef<FormatElement *> elements) {
+  using ParsingBranch = SmallVector<ArrayRef<FormatElement *>, 2>;
+  std::vector<ParsingBranch> parsingPaths;
+  parsingPaths.reserve(elements.size());
 
-    // Check for an attribute with a constant type builder, followed by a `:`.
-    auto *prevAttr = dyn_cast<AttributeVariable>(element);
-    if (!prevAttr || prevAttr->getTypeBuilder())
+  auto *it = elements.begin(), *e = elements.end();
+  auto *start = it;
+  while (it!= e) {
+    auto *optional = dyn_cast<OptionalElement>(*it);
+    // Iterate until an optional element is found.
+    if (!optional) {
+      ++it;
       continue;
-
-    // Check the next iterator within the stack for literal elements.
-    for (auto &nextItPair : iteratorStack) {
-      ElementsItT nextIt = nextItPair.first, nextE = nextItPair.second;
-      for (; nextIt != nextE; ++nextIt) {
-        // Skip any trailing whitespace, attribute dictionaries, or optional
-        // groups.
-        if (isa<WhitespaceElement>(*nextIt) ||
-            isa<AttrDictDirective>(*nextIt) || isa<OptionalElement>(*nextIt))
+    }
+    // Push the current elements.
+    if (it != start) {
+      parsingPaths.emplace_back().push_back(
+          {start, static_cast<unsigned>(std::distance(start, it))});
+    }
+    // Skip the optional element.
+    ++it;
+    start = it;
+    // Push its children.
+    SmallVectorImpl<ArrayRef<FormatElement *>> &path =
+        parsingPaths.emplace_back();
+    path.push_back(optional->getThenElements());
+    if (!optional->getElseElements().empty())
+     path.push_back(optional->getElseElements());
+    // Push an empty branch.
+    path.push_back({});
+  }
+  // Since we iterated to the end, push any remaining elements.
+  if (it != start) {
+    parsingPaths.emplace_back().push_back(
+        {start, static_cast<unsigned>(std::distance(start, it))});
+  }
+
+  assert(!parsingPaths.empty() && !parsingPaths.front().empty() &&
+         "unexpected empty assembly format");
+  // Iterate until we find an attribute element. Then, starting from that
+  // position, check until the end of the current path segment, then move on to
+  // the next segments. Check all branches of subsequent segments.
+  for (auto pathIt = parsingPaths.begin(), pathEnd = parsingPaths.end();
+       pathIt != pathEnd; ++pathIt) {
+    for (auto *branchIt = pathIt->begin(), *branchEnd = pathIt->end();
+         branchIt != branchEnd; ++branchIt) {
+      for (auto *elIt = branchIt->begin(), *elEnd = branchIt->end();
+           elIt != elEnd; ++elIt) {
+        // Check attributes that lack a type builder.
+        auto *attr = dyn_cast<AttributeVariable>(*elIt);
+        if (!attr || attr->getTypeBuilder())
+          continue;
+        auto checkPath =
+            [&](iterator_range<ArrayRef<FormatElement *>::iterator> path) {
+              return checkPathSegmentForColon(
+                  [&](const Twine &msg) { return emitError(loc, msg); },
+                  attr->getVar()->name, path);
+            };
+        if (Optional<LogicalResult> result =
+                checkPath(llvm::make_range(std::next(elIt), elEnd)))
+          return *result;
+        // Check all possible parsing paths starting from the next segment.
+        for (auto secondPathIt = std::next(pathIt); secondPathIt != pathEnd;
+             ++secondPathIt) {
+          bool reachedEnd = false;
+          for (ArrayRef<FormatElement *> path : *secondPathIt) {
+            if (Optional<LogicalResult> result = checkPath(path)) {
+              if (failed(*result))
+                return failure();
               continue;
+            }
+            reachedEnd = true;
+          }
+          if (!reachedEnd)
+            return success();
+        }
+      }
+    }
+  }
 
-        // We are only interested in `:` literals.
-        auto *literal = dyn_cast<LiteralElement>(*nextIt);
-        if (!literal || literal->getSpelling() != ":")
-          break;
+  return success();
+}
 
-        // TODO: Use the location of the literal element itself.
+LogicalResult
+OpFormatParser::verifyOIListElements(SMLoc loc,
+                                     ArrayRef<FormatElement *> elements) {
+  // Check that all of the successors are within the format.
+  SmallVector<StringRef> prohibitedLiterals;
+  for (FormatElement *it : elements) {
+    if (auto *oilist = dyn_cast<OIListElement>(it)) {
+      if (!prohibitedLiterals.empty()) {
+        // We just saw an oilist element in last iteration. Literals should not
+        // match.
+        for (LiteralElement *literal : oilist->getLiteralElements()) {
+          if (find(prohibitedLiterals, literal->getSpelling()) !=
+              prohibitedLiterals.end()) {
             return emitError(
-            loc, llvm::formatv("format ambiguity caused by `:` literal found "
-                               "after attribute `{0}` which does not have "
-                               "a buildable type",
-                               prevAttr->getVar()->name));
+                loc, "format ambiguity because " + literal->getSpelling() +
+                         " is used in two adjacent oilist elements.");
+          }
         }
       }
+      for (LiteralElement *literal : oilist->getLiteralElements())
+        prohibitedLiterals.push_back(literal->getSpelling());
+    } else if (auto *literal = dyn_cast<LiteralElement>(it)) {
+      if (find(prohibitedLiterals, literal->getSpelling()) !=
+          prohibitedLiterals.end()) {
+        return emitError(
+            loc,
+            "format ambiguity because " + literal->getSpelling() +
+                " is used both in oilist element and the adjacent literal.");
+      }
+      prohibitedLiterals.clear();
+    } else {
+      prohibitedLiterals.clear();
+    }
   }
-  iteratorStack.pop_back();
   return success();
 }
 
 LogicalResult OpFormatParser::verifyOperands(
     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;
if (!seenSuccessors.count(&successor)) {
                                   successor.name + "', not found",
                               "suggest adding a '$" + successor.name +
                                   "' directive to the custom assembly format");
     }
   }
   return success();
 }
 
-LogicalResult
-OpFormatParser::verifyOIListElements(SMLoc loc,
-                                     ArrayRef<FormatElement *> elements) {
-  // Check that all of the successors are within the format.
-  SmallVector<StringRef> prohibitedLiterals;
-  for (FormatElement *it : elements) {
-    if (auto *oilist = dyn_cast<OIListElement>(it)) {
-      if (!prohibitedLiterals.empty()) {
-        // We just saw an oilist element in last iteration. Literals should not
-        // match.
-        for (LiteralElement *literal : oilist->getLiteralElements()) {
-          if (find(prohibitedLiterals, literal->getSpelling()) !=
-              prohibitedLiterals.end()) {
-            return emitError(
-                loc, "format ambiguity because " + literal->getSpelling() +
-                         " is used in two adjacent oilist elements.");
-          }
-        }
-      }
-      for (LiteralElement *literal : oilist->getLiteralElements())
-        prohibitedLiterals.push_back(literal->getSpelling());
-    } else if (auto *literal = dyn_cast<LiteralElement>(it)) {
-      if (find(prohibitedLiterals, literal->getSpelling()) !=
-          prohibitedLiterals.end()) {
-        return emitError(
-            loc,
-            "format ambiguity because " + literal->getSpelling() +
-                " is used both in oilist element and the adjacent literal.");
-      }
-      prohibitedLiterals.clear();
-    } else {
-      prohibitedLiterals.clear();
-    }
-  }
-  return success();
-}
-
 void OpFormatParser::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.
     ConstArgument arg = findSeenArg(values[i]);
     if (!arg)
       continue;