[OpAsmParser] Add a parseCommaSeparatedList helper and beef up Delimeter.

[lattner]

Summary

Lots of custom ops have hand-rolled comma-delimited parsing loops, as does
the MLIR parser itself. Provides a standard interface for doing this that
is less error prone and less boilerplate.

While here, extend Delimiter to support <> and {} delimited sequences as
well (I have a use for <> in CIRCT specifically).

[lattner]

Note that I updated a bunch of random examples in the tree but there are many more that could adopt this as well.

[lattner]

mlir/lib/Parser/TypeParser.cpp

564

AFAICT, this was an outright bug that would lead to rejecting malformed input without emitting a diagnostic.

[rriddle]

Looks much cleaner, thanks!

mlir/lib/Parser/Parser.cpp

1361

Why the move here?

mlir/lib/Parser/TypeParser.cpp

172–173

Now that parseStrideList actually emits errors in all cases, we should drop this error.

564

It looks like line73 also emits an error, which I guess covers this but can also result in a double error. We should remove the error on that line.

[lattner]

Improvements pointed out by River

mlir/lib/Parser/Parser.cpp

1361

Great catch, I was using std::function, but moved to function_ref later. function_ref doesn't need this. Removed from the patch, thanks!

mlir/lib/Parser/TypeParser.cpp

564

Aha, cool I cleaned this up, good catch.

[mehdi_amini]

Nice!

[lattner]

Remove one more unnecessary move.

[lattner]

Thank you for the speedy reviews!

mlir/include/mlir/IR/OpImplementation.h

OptionalParseResult parseOptionalInteger(IntT &result) {
     // zero for non-negated integers.
     result =
         (IntT)uintResult.sextOrTrunc(sizeof(IntT) * CHAR_BIT).getLimitedValue();
     if (APInt(uintResult.getBitWidth(), result) != uintResult)
       return emitError(loc, "integer value too large");
     return success();
   }
 
+  /// These are the supported delimiters around operand lists and region
+  /// argument lists, used by parseOperandList and parseRegionArgumentList.
+  enum class Delimiter {
+    /// Zero or more operands with no delimiters.
+    None,
+    /// Parens surrounding zero or more operands.
+    Paren,
+    /// Square brackets surrounding zero or more operands.
+    Square,
+    /// <> brackets surrounding zero or more operands.
+    LessGreater,
+    /// {} brackets surrounding zero or more operands.
+    Braces,
+    /// Parens supporting zero or more operands, or nothing.
+    OptionalParen,
+    /// Square brackets supporting zero or more ops, or nothing.
+    OptionalSquare,
+    /// <> brackets supporting zero or more ops, or nothing.
+    OptionalLessGreater,
+    /// {} brackets surrounding zero or more operands, or nothing.
+    OptionalBraces,
+  };
+
+  /// Parse a list of comma-separated items with an optional delimiter.  If a
+  /// delimiter is provided, then an empty list is allowed.  If not, then at
+  /// least one element will be parsed.
+  ///
+  /// contextMessage is an optional message appended to "expected '('" sorts of
+  /// diagnostics when parsing the delimeters.
+  virtual ParseResult
+  parseCommaSeparatedList(Delimiter delimiter,
+                          function_ref<ParseResult()> parseElementFn,
+                          StringRef contextMessage = StringRef()) = 0;
+
+  /// Parse a comma separated list of elements that must have at least one entry
+  /// in it.
+  ParseResult
+  parseCommaSeparatedList(function_ref<ParseResult()> parseElementFn) {
+    return parseCommaSeparatedList(Delimiter::None, parseElementFn);
+  }
+
   //===--------------------------------------------------------------------===//
   // Attribute Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse an arbitrary attribute of a given type and return it in result.
   virtual ParseResult parseAttribute(Attribute &result, Type type = {}) = 0;
 
   /// Parse an attribute of a specific kind and type.
public:
   };
 
   /// Parse a single operand.
   virtual ParseResult parseOperand(OperandType &result) = 0;
 
   /// Parse a single operand if present.
   virtual OptionalParseResult parseOptionalOperand(OperandType &result) = 0;
 
-  /// These are the supported delimiters around operand lists and region
-  /// argument lists, used by parseOperandList and parseRegionArgumentList.
-  enum class Delimiter {
-    /// Zero or more operands with no delimiters.
-    None,
-    /// Parens surrounding zero or more operands.
-    Paren,
-    /// Square brackets surrounding zero or more operands.
-    Square,
-    /// Parens supporting zero or more operands, or nothing.
-    OptionalParen,
-    /// Square brackets supporting zero or more ops, or nothing.
-    OptionalSquare,
-  };
-
   /// Parse zero or more SSA comma-separated operand references with a specified
   /// surrounding delimiter, and an optional required operand count.
   virtual ParseResult
   parseOperandList(SmallVectorImpl<OperandType> &result,
                    int requiredOperandCount = -1,
                    Delimiter delimiter = Delimiter::None) = 0;
   ParseResult parseOperandList(SmallVectorImpl<OperandType> &result,
                                Delimiter delimiter) {

mlir/lib/Dialect/Async/IR/Async.cpp

static void print(OpAsmPrinter &p, ExecuteOp op) {
   p.printRegion(op.body(), /*printEntryBlockArgs=*/false);
 }
 
 static ParseResult parseExecuteOp(OpAsmParser &parser, OperationState &result) {
   MLIRContext *ctx = result.getContext();
 
   // Sizes of parsed variadic operands, will be updated below after parsing.
   int32_t numDependencies = 0;
-  int32_t numOperands = 0;
 
   auto tokenTy = TokenType::get(ctx);
 
   // Parse dependency tokens.
   if (succeeded(parser.parseOptionalLSquare())) {
     SmallVector<OpAsmParser::OperandType, 4> tokenArgs;
     if (parser.parseOperandList(tokenArgs) ||
         parser.resolveOperands(tokenArgs, tokenTy, result.operands) ||
         parser.parseRSquare())
       return failure();
 
     numDependencies = tokenArgs.size();
   }
 
   // Parse async value operands (%value as %unwrapped : !async.value<!type>).
   SmallVector<OpAsmParser::OperandType, 4> valueArgs;
   SmallVector<OpAsmParser::OperandType, 4> unwrappedArgs;
   SmallVector<Type, 4> valueTypes;
   SmallVector<Type, 4> unwrappedTypes;
 
-  if (succeeded(parser.parseOptionalLParen())) {
-    auto argsLoc = parser.getCurrentLocation();
-
-    // Parse a single instance of `%value as %unwrapped : !async.value<!type>`.
-    auto parseAsyncValueArg = [&]() -> ParseResult {
-      if (parser.parseOperand(valueArgs.emplace_back()) ||
-          parser.parseKeyword("as") ||
-          parser.parseOperand(unwrappedArgs.emplace_back()) ||
-          parser.parseColonType(valueTypes.emplace_back()))
-        return failure();
 
-      auto valueTy = valueTypes.back().dyn_cast<ValueType>();
-      unwrappedTypes.emplace_back(valueTy ? valueTy.getValueType() : Type());
 
-      return success();
-    };
 
-    // If the next token is `)` skip async value arguments parsing.
-    if (failed(parser.parseOptionalRParen())) {
-      do {
-        if (parseAsyncValueArg())
-          return failure();
-      } while (succeeded(parser.parseOptionalComma()));
-
-      if (parser.parseRParen() ||
-          parser.resolveOperands(valueArgs, valueTypes, argsLoc,
-                                 result.operands))
-        return failure();
-    }
+  auto argsLoc = parser.getCurrentLocation();
+  if (parser.parseCommaSeparatedList(OpAsmParser::Delimiter::OptionalParen,
+                                     parseAsyncValueArg) ||
+      parser.resolveOperands(valueArgs, valueTypes, argsLoc, result.operands))
 
-    numOperands = valueArgs.size();
-  }
+  int32_t numOperands = valueArgs.size();
 
   // Add derived `operand_segment_sizes` attribute based on parsed operands.
   auto operandSegmentSizes = DenseIntElementsAttr::get(
       VectorType::get({2}, parser.getBuilder().getI32Type()),
       {numDependencies, numOperands});
   result.addAttribute(kOperandSegmentSizesAttr, operandSegmentSizes);
 
   // Parse the types of results returned from the async execute op.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

 /// operand-and-type-list ::= `(` ssa-id-and-type-list `)`
 /// ssa-id-and-type-list ::= ssa-id-and-type |
 ///                          ssa-id-and-type `,` ssa-id-and-type-list
 /// ssa-id-and-type ::= ssa-id `:` type
 static ParseResult
 parseOperandAndTypeList(OpAsmParser &parser,
                         SmallVectorImpl<OpAsmParser::OperandType> &operands,
                         SmallVectorImpl<Type> &types) {
-  if (parser.parseLParen())
-    return failure();
-
-  do {
+  return parser.parseCommaSeparatedList(
+      OpAsmParser::Delimiter::Paren, [&]() -> ParseResult {
         OpAsmParser::OperandType operand;
         Type type;
         if (parser.parseOperand(operand) || parser.parseColonType(type))
           return failure();
         operands.push_back(operand);
         types.push_back(type);
-  } while (succeeded(parser.parseOptionalComma()));
-
-  if (parser.parseRParen())
-    return failure();
-
         return success();
+      });
 }
 
 /// Parse an allocate clause with allocators and a list of operands with types.
 ///
 /// operand-and-type-list ::= `(` allocate-operand-list `)`
 /// allocate-operand-list :: = allocate-operand |
 ///                            allocator-operand `,` allocate-operand-list
 /// allocate-operand :: = ssa-id-and-type -> ssa-id-and-type
 /// ssa-id-and-type ::= ssa-id `:` type
 static ParseResult parseAllocateAndAllocator(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::OperandType> &operandsAllocate,
     SmallVectorImpl<Type> &typesAllocate,
     SmallVectorImpl<OpAsmParser::OperandType> &operandsAllocator,
     SmallVectorImpl<Type> &typesAllocator) {
-  if (parser.parseLParen())
-    return failure();
 
-  do {
+  return parser.parseCommaSeparatedList(
+      OpAsmParser::Delimiter::Paren, [&]() -> ParseResult {
         OpAsmParser::OperandType operand;
         Type type;
-
         if (parser.parseOperand(operand) || parser.parseColonType(type))
           return failure();
         operandsAllocator.push_back(operand);
         typesAllocator.push_back(type);
         if (parser.parseArrow())
           return failure();
         if (parser.parseOperand(operand) || parser.parseColonType(type))
           return failure();
 
         operandsAllocate.push_back(operand);
         typesAllocate.push_back(type);
-  } while (succeeded(parser.parseOptionalComma()));
-
-  if (parser.parseRParen())
-    return failure();
-
         return success();
+      });
 }
 
 static LogicalResult verifyParallelOp(ParallelOp op) {
   if (op.allocate_vars().size() != op.allocators_vars().size())
     return op.emitError(
         "expected equal sizes for allocate and allocator variables");
   return success();
 }

mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp

static ParseResult parseEntryPointOp(OpAsmParser &parser,
   FlatSymbolRefAttr fn;
   if (parseEnumStrAttr(execModel, parser, state) ||
       parser.parseAttribute(fn, Type(), kFnNameAttrName, state.attributes)) {
     return failure();
   }
 
   if (!parser.parseOptionalComma()) {
     // Parse the interface variables
-    do {
+    if (parser.parseCommaSeparatedList([&]() -> ParseResult {
           // The name of the interface variable attribute isnt important
-      auto attrName = "var_symbol";
           FlatSymbolRefAttr var;
           NamedAttrList attrs;
-      if (parser.parseAttribute(var, Type(), attrName, attrs)) {
+          if (parser.parseAttribute(var, Type(), "var_symbol", attrs))
             return failure();
-      }
           interfaceVars.push_back(var);
-    } while (!parser.parseOptionalComma());
+          return success();
+        }))
+      return failure();
   }
   state.addAttribute(kInterfaceAttrName,
                      parser.getBuilder().getArrayAttr(interfaceVars));
   return success();
 }
 
 static void print(spirv::EntryPointOp entryPointOp, OpAsmPrinter &printer) {
   printer << " \"" << stringifyExecutionModel(entryPointOp.execution_model())

mlir/lib/Dialect/StandardOps/IR/Ops.cpp

 static ParseResult parseSwitchOpCases(
     OpAsmParser &parser, Type &flagType, Block *&defaultDestination,
     SmallVectorImpl<OpAsmParser::OperandType> &defaultOperands,
     SmallVectorImpl<Type> &defaultOperandTypes,
     DenseIntElementsAttr &caseValues,
     SmallVectorImpl<Block *> &caseDestinations,
     SmallVectorImpl<SmallVector<OpAsmParser::OperandType>> &caseOperands,
     SmallVectorImpl<SmallVector<Type>> &caseOperandTypes) {
-  if (failed(parser.parseKeyword("default")) || failed(parser.parseColon()) ||
-      failed(parser.parseSuccessor(defaultDestination)))
+  if (parser.parseKeyword("default") || parser.parseColon() ||
+      parser.parseSuccessor(defaultDestination))
     return failure();
   if (succeeded(parser.parseOptionalLParen())) {
-    if (failed(parser.parseRegionArgumentList(defaultOperands)) ||
-        failed(parser.parseColonTypeList(defaultOperandTypes)) ||
-        failed(parser.parseRParen()))
+    if (parser.parseRegionArgumentList(defaultOperands) ||
+        parser.parseColonTypeList(defaultOperandTypes) || parser.parseRParen())
       return failure();
   }
 
   SmallVector<APInt> values;
   unsigned bitWidth = flagType.getIntOrFloatBitWidth();
   while (succeeded(parser.parseOptionalComma())) {
     int64_t value = 0;
     if (failed(parser.parseInteger(value)))

mlir/lib/Parser/AffineParser.cpp

ParseResult AffineParser::parseIdentifierDefinition(AffineExpr idExpr) {
   consumeToken(Token::bare_identifier);
 
   dimsAndSymbols.push_back({name, idExpr});
   return success();
 }
 
 /// Parse the list of dimensional identifiers to an affine map.
 ParseResult AffineParser::parseDimIdList(unsigned &numDims) {
-  if (parseToken(Token::l_paren,
-                 "expected '(' at start of dimensional identifiers list")) {
-    return failure();
-  }
-
   auto parseElt = [&]() -> ParseResult {
     auto dimension = getAffineDimExpr(numDims++, getContext());
     return parseIdentifierDefinition(dimension);
   };
-  return parseCommaSeparatedListUntil(Token::r_paren, parseElt);
+  return parseCommaSeparatedList(Delimiter::Paren, parseElt,
+                                 " in dimensional identifier list");
 }
 
 /// Parse the list of symbolic identifiers to an affine map.
 ParseResult AffineParser::parseSymbolIdList(unsigned &numSymbols) {
-  consumeToken(Token::l_square);
   auto parseElt = [&]() -> ParseResult {
     auto symbol = getAffineSymbolExpr(numSymbols++, getContext());
     return parseIdentifierDefinition(symbol);
   };
-  return parseCommaSeparatedListUntil(Token::r_square, parseElt);
+  return parseCommaSeparatedList(Delimiter::Square, parseElt,
+                                 " in symbol list");
 }
 
 /// Parse the list of symbolic identifiers to an affine map.
 ParseResult
 AffineParser::parseDimAndOptionalSymbolIdList(unsigned &numDims,
                                               unsigned &numSymbols) {
   if (parseDimIdList(numDims)) {
     return failure();
ParseResult AffineParser::parseAffineMapOrIntegerSetInline(AffineMap &map,
 
   return failure();
 }
 
 /// Parse an AffineMap where the dim and symbol identifiers are SSA ids.
 ParseResult
 AffineParser::parseAffineMapOfSSAIds(AffineMap &map,
                                      OpAsmParser::Delimiter delimiter) {
-  Token::Kind rightToken;
-  switch (delimiter) {
-  case OpAsmParser::Delimiter::Square:
-    if (parseToken(Token::l_square, "expected '['"))
-      return failure();
-    rightToken = Token::r_square;
-    break;
-  case OpAsmParser::Delimiter::Paren:
-    if (parseToken(Token::l_paren, "expected '('"))
-      return failure();
-    rightToken = Token::r_paren;
-    break;
-  default:
-    return emitError("unexpected delimiter");
-  }
 
   SmallVector<AffineExpr, 4> exprs;
   auto parseElt = [&]() -> ParseResult {
     auto elt = parseAffineExpr();
     exprs.push_back(elt);
     return elt ? success() : failure();
   };
 
   // Parse a multi-dimensional affine expression (a comma-separated list of
   // 1-d affine expressions); the list can be empty. Grammar:
   // multi-dim-affine-expr ::= `(` `)`
   //                         | `(` affine-expr (`,` affine-expr)* `)`
-  if (parseCommaSeparatedListUntil(rightToken, parseElt,
-                                   /*allowEmptyList=*/true))
+  if (parseCommaSeparatedList(delimiter, parseElt, " in affine map"))
     return failure();
+
   // Parsed a valid affine map.
   map = AffineMap::get(numDimOperands, dimsAndSymbols.size() - numDimOperands,
                        exprs, getContext());
   return success();
 }
 
 /// Parse an AffineExpr where the dim and symbol identifiers are SSA ids.
 ParseResult AffineParser::parseAffineExprOfSSAIds(AffineExpr &expr) {
   expr = parseAffineExpr();
   return success(expr != nullptr);
 }
 
 /// Parse the range and sizes affine map definition inline.
 ///
 ///  affine-map ::= dim-and-symbol-id-lists `->` multi-dim-affine-expr
 ///
 ///  multi-dim-affine-expr ::= `(` `)`
 ///  multi-dim-affine-expr ::= `(` affine-expr (`,` affine-expr)* `)`
 AffineMap AffineParser::parseAffineMapRange(unsigned numDims,
                                             unsigned numSymbols) {
-  parseToken(Token::l_paren, "expected '(' at start of affine map range");
-
   SmallVector<AffineExpr, 4> exprs;
   auto parseElt = [&]() -> ParseResult {
     auto elt = parseAffineExpr();
     ParseResult res = elt ? success() : failure();
     exprs.push_back(elt);
     return res;
   };
 
   // Parse a multi-dimensional affine expression (a comma-separated list of
   // 1-d affine expressions). Grammar:
   // multi-dim-affine-expr ::= `(` `)`
   //                         | `(` affine-expr (`,` affine-expr)* `)`
-  if (parseCommaSeparatedListUntil(Token::r_paren, parseElt, true))
+  if (parseCommaSeparatedList(Delimiter::Paren, parseElt,
+                              " in affine map range"))
     return AffineMap();
 
   // Parsed a valid affine map.
   return AffineMap::get(numDims, numSymbols, exprs, getContext());
 }
 
 /// Parse an affine constraint.
 ///  affine-constraint ::= affine-expr `>=` `0`

 ///  integer-set-inline
 ///                ::= dim-and-symbol-id-lists `:`
 ///                '(' affine-constraint-conjunction? ')'
 ///  affine-constraint-conjunction ::= affine-constraint (`,`
 ///                                       affine-constraint)*
 ///
 IntegerSet AffineParser::parseIntegerSetConstraints(unsigned numDims,
                                                     unsigned numSymbols) {
-  if (parseToken(Token::l_paren,
-                 "expected '(' at start of integer set constraint list"))
-    return IntegerSet();
-
   SmallVector<AffineExpr, 4> constraints;
   SmallVector<bool, 4> isEqs;
   auto parseElt = [&]() -> ParseResult {
     bool isEq;
     auto elt = parseAffineConstraint(&isEq);
     ParseResult res = elt ? success() : failure();
     if (elt) {
       constraints.push_back(elt);
       isEqs.push_back(isEq);
     }
     return res;
   };
 
   // Parse a list of affine constraints (comma-separated).
-  if (parseCommaSeparatedListUntil(Token::r_paren, parseElt, true))
+  if (parseCommaSeparatedList(Delimiter::Paren, parseElt,
+                              " in integer set constraint list"))
     return IntegerSet();
 
   // If no constraints were parsed, then treat this as a degenerate 'true' case.
   if (constraints.empty()) {
     /* 0 == 0 */
     auto zero = getAffineConstantExpr(0, getContext());
     return IntegerSet::get(numDims, numSymbols, zero, true);
   }

mlir/lib/Parser/AttributeParser.cpp

if (parseToken(Token::less, "expected '<' in integer set") ||
         parseIntegerSetReference(set) ||
         parseToken(Token::greater, "expected '>' in integer set"))
       return Attribute();
     return IntegerSetAttr::get(set);
   }
 
   // Parse an array attribute.
   case Token::l_square: {
-    consumeToken(Token::l_square);
-
     SmallVector<Attribute, 4> elements;
     auto parseElt = [&]() -> ParseResult {
       elements.push_back(parseAttribute());
       return elements.back() ? success() : failure();
     };
 
-    if (parseCommaSeparatedListUntil(Token::r_square, parseElt))
+    if (parseCommaSeparatedList(Delimiter::Square, parseElt))
       return nullptr;
     return builder.getArrayAttr(elements);
   }
 
   // Parse a boolean attribute.
   case Token::kw_false:
     consumeToken(Token::kw_false);
     return builder.getBoolAttr(false);

 
 /// Attribute dictionary.
 ///
 ///   attribute-dict ::= `{` `}`
 ///                    | `{` attribute-entry (`,` attribute-entry)* `}`
 ///   attribute-entry ::= (bare-id | string-literal) `=` attribute-value
 ///
 ParseResult Parser::parseAttributeDict(NamedAttrList &attributes) {
-  if (parseToken(Token::l_brace, "expected '{' in attribute dictionary"))
-    return failure();
-
   llvm::SmallDenseSet<Identifier> seenKeys;
   auto parseElt = [&]() -> ParseResult {
     // The name of an attribute can either be a bare identifier, or a string.
     Optional<Identifier> nameId;
     if (getToken().is(Token::string))
       nameId = builder.getIdentifier(getToken().getStringValue());
     else if (getToken().isAny(Token::bare_identifier, Token::inttype) ||
              getToken().isKeyword())
auto parseElt = [&]() -> ParseResult {
 
     auto attr = parseAttribute();
     if (!attr)
       return failure();
     attributes.push_back({*nameId, attr});
     return success();
   };
 
-  if (parseCommaSeparatedListUntil(Token::r_brace, parseElt))
+  if (parseCommaSeparatedList(Delimiter::Braces, parseElt,
+                              " in attribute dictionary"))
     return failure();
 
   return success();
 }
 
 /// Parse a float attribute.
 Attribute Parser::parseFloatAttr(Type type, bool isNegative) {
   auto val = getToken().getFloatingPointValue();

 
 /// Parse a list of either lists or elements, returning the dimensions of the
 /// parsed sub-tensors in dims. For example:
 ///   parseList([1, 2, 3]) -> Success, [3]
 ///   parseList([[1, 2], [3, 4]]) -> Success, [2, 2]
 ///   parseList([[1, 2], 3]) -> Failure
 ///   parseList([[1, [2, 3]], [4, [5]]]) -> Failure
 ParseResult TensorLiteralParser::parseList(SmallVectorImpl<int64_t> &dims) {
-  p.consumeToken(Token::l_square);
-
   auto checkDims = [&](const SmallVectorImpl<int64_t> &prevDims,
                        const SmallVectorImpl<int64_t> &newDims) -> ParseResult {
     if (prevDims == newDims)
       return success();
     return p.emitError("tensor literal is invalid; ranks are not consistent "
                        "between elements");
   };
 
   bool first = true;
   SmallVector<int64_t, 4> newDims;
   unsigned size = 0;
-  auto parseCommaSeparatedList = [&]() -> ParseResult {
+  auto parseOneElement = [&]() -> ParseResult {
     SmallVector<int64_t, 4> thisDims;
     if (p.getToken().getKind() == Token::l_square) {
       if (parseList(thisDims))
         return failure();
     } else if (parseElement()) {
       return failure();
     }
     ++size;
     if (!first)
       return checkDims(newDims, thisDims);
     newDims = thisDims;
     first = false;
     return success();
   };
-  if (p.parseCommaSeparatedListUntil(Token::r_square, parseCommaSeparatedList))
+  if (p.parseCommaSeparatedList(Parser::Delimiter::Square, parseOneElement))
     return failure();
 
   // Return the sublists' dimensions with 'size' prepended.
   dims.clear();
   dims.push_back(size);
   dims.append(newDims.begin(), newDims.end());
   return success();
 }

mlir/lib/Parser/LocationParser.cpp

ParseResult Parser::parseFusedLocation(LocationAttr &loc) {
   auto parseElt = [&] {
     LocationAttr newLoc;
     if (parseLocationInstance(newLoc))
       return failure();
     locations.push_back(newLoc);
     return success();
   };
 
-  if (parseToken(Token::l_square, "expected '[' in fused location") ||
-      parseCommaSeparatedList(parseElt) ||
-      parseToken(Token::r_square, "expected ']' in fused location"))
+  if (parseCommaSeparatedList(Delimiter::Square, parseElt,
+                              " in fused location"))
     return failure();
 
   // Return the fused location.
   loc = FusedLoc::get(locations, metadata, getContext());
   return success();
 }
 
 ParseResult Parser::parseNameOrFileLineColLocation(LocationAttr &loc) {

mlir/lib/Parser/Parser.h

 // Parser
 //===----------------------------------------------------------------------===//
 
 /// This class implement support for parsing global entities like attributes and
 /// types. It is intended to be subclassed by specialized subparsers that
 /// include state.
 class Parser {
 public:
+  using Delimiter = OpAsmParser::Delimiter;
+
   Builder builder;
 
   Parser(ParserState &state) : builder(state.context), state(state) {}
 
   // Helper methods to get stuff from the parser-global state.
   ParserState &getState() const { return state; }
   MLIRContext *getContext() const { return state.context; }
   const llvm::SourceMgr &getSourceMgr() { return state.lex.getSourceMgr(); }
 
   /// Parse a comma-separated list of elements up until the specified end token.
   ParseResult
   parseCommaSeparatedListUntil(Token::Kind rightToken,
                                function_ref<ParseResult()> parseElement,
                                bool allowEmptyList = true);
 
+  /// Parse a list of comma-separated items with an optional delimiter.  If a
+  /// delimiter is provided, then an empty list is allowed.  If not, then at
+  /// least one element will be parsed.
+  ParseResult
+  parseCommaSeparatedList(Delimiter delimiter,
+                          function_ref<ParseResult()> parseElementFn,
+                          StringRef contextMessage = StringRef());
+
   /// Parse a comma separated list of elements that must have at least one entry
   /// in it.
-  ParseResult parseCommaSeparatedList(function_ref<ParseResult()> parseElement);
+  ParseResult
+  parseCommaSeparatedList(function_ref<ParseResult()> parseElementFn) {
+    return parseCommaSeparatedList(Delimiter::None, parseElementFn);
+  }
 
   ParseResult parsePrettyDialectSymbolName(StringRef &prettyName);
 
   // We have two forms of parsing methods - those that return a non-null
   // pointer on success, and those that return a ParseResult to indicate whether
   // they returned a failure.  The second class fills in by-reference arguments
   // as the results of their action.
 
ParseResult parseAffineMapOrIntegerSetReference(AffineMap &map,
                                                   IntegerSet &set);
   ParseResult parseAffineMapReference(AffineMap &map);
   ParseResult parseIntegerSetReference(IntegerSet &set);
 
   /// Parse an AffineMap where the dim and symbol identifiers are SSA ids.
   ParseResult
   parseAffineMapOfSSAIds(AffineMap &map,
                          function_ref<ParseResult(bool)> parseElement,
-                         OpAsmParser::Delimiter delimiter);
+                         Delimiter delimiter);
 
   /// Parse an AffineExpr where dim and symbol identifiers are SSA ids.
   ParseResult
   parseAffineExprOfSSAIds(AffineExpr &expr,
                           function_ref<ParseResult(bool)> parseElement);
 
 protected:
   /// The Parser is subclassed and reinstantiated.  Do not add additional
   /// non-trivial state here, add it to the ParserState class.
   ParserState &state;
 };
 } // end namespace detail
 } // end namespace mlir
 
 #endif // MLIR_LIB_PARSER_PARSER_H

mlir/lib/Parser/Parser.cpp

 using llvm::MemoryBuffer;
 using llvm::SMLoc;
 using llvm::SourceMgr;
 
 //===----------------------------------------------------------------------===//
 // Parser
 //===----------------------------------------------------------------------===//
 
-/// Parse a comma separated list of elements that must have at least one entry
-/// in it.
+/// Parse a list of comma-separated items with an optional delimiter.  If a
+/// delimiter is provided, then an empty list is allowed.  If not, then at
+/// least one element will be parsed.
 ParseResult
-Parser::parseCommaSeparatedList(function_ref<ParseResult()> parseElement) {
+Parser::parseCommaSeparatedList(Delimiter delimiter,
+                                function_ref<ParseResult()> parseElementFn,
+                                StringRef contextMessage) {
+  switch (delimiter) {
+  case Delimiter::None:
+    break;
+  case Delimiter::OptionalParen:
+    if (getToken().isNot(Token::l_paren))
+      return success();
+    LLVM_FALLTHROUGH;
+  case Delimiter::Paren:
+    if (parseToken(Token::l_paren, "expected '('" + contextMessage))
+      return failure();
+    // Check for empty list.
+    if (consumeIf(Token::r_paren))
+      return success();
+    break;
+  case Delimiter::OptionalLessGreater:
+    // Check for absent list.
+    if (getToken().isNot(Token::less))
+      return success();
+    LLVM_FALLTHROUGH;
+  case Delimiter::LessGreater:
+    if (parseToken(Token::less, "expected '<'" + contextMessage))
+      return success();
+    // Check for empty list.
+    if (consumeIf(Token::greater))
+      return success();
+    break;
+  case Delimiter::OptionalSquare:
+    if (getToken().isNot(Token::l_square))
+      return success();
+    LLVM_FALLTHROUGH;
+  case Delimiter::Square:
+    if (parseToken(Token::l_square, "expected '['" + contextMessage))
+      return failure();
+    // Check for empty list.
+    if (consumeIf(Token::r_square))
+      return success();
+    break;
+  case Delimiter::OptionalBraces:
+    if (getToken().isNot(Token::l_brace))
+      return success();
+    LLVM_FALLTHROUGH;
+  case Delimiter::Braces:
+    if (parseToken(Token::l_brace, "expected '{'" + contextMessage))
+      return failure();
+    // Check for empty list.
+    if (consumeIf(Token::r_brace))
+      return success();
+    break;
+  }
+
   // Non-empty case starts with an element.
-  if (parseElement())
+  if (parseElementFn())
     return failure();
 
   // Otherwise we have a list of comma separated elements.
   while (consumeIf(Token::comma)) {
-    if (parseElement())
+    if (parseElementFn())
       return failure();
   }
+
+  switch (delimiter) {
+  case Delimiter::None:
     return success();
+  case Delimiter::OptionalParen:
+  case Delimiter::Paren:
+    return parseToken(Token::r_paren, "expected ')'" + contextMessage);
+  case Delimiter::OptionalLessGreater:
+  case Delimiter::LessGreater:
+    return parseToken(Token::greater, "expected '>'" + contextMessage);
+  case Delimiter::OptionalSquare:
+  case Delimiter::Square:
+    return parseToken(Token::r_square, "expected ']'" + contextMessage);
+  case Delimiter::OptionalBraces:
+  case Delimiter::Braces:
+    return parseToken(Token::r_brace, "expected '}'" + contextMessage);
+  }
+  llvm_unreachable("Unknown delimiter");
 }
 
 /// Parse a comma-separated list of elements, terminated with an arbitrary
 /// token.  This allows empty lists if allowEmptyList is true.
 ///
 ///   abstract-list ::= rightToken                  // if allowEmptyList == true
 ///   abstract-list ::= element (',' element)* rightToken
 ///
ParseResult parseOptionalStar() override {
     return success(parser.consumeIf(Token::star));
   }
 
   /// Parse an optional integer value from the stream.
   OptionalParseResult parseOptionalInteger(APInt &result) override {
     return parser.parseOptionalInteger(result);
   }
 
+  /// Parse a list of comma-separated items with an optional delimiter.  If a
+  /// delimiter is provided, then an empty list is allowed.  If not, then at
+  /// least one element will be parsed.
+  ParseResult parseCommaSeparatedList(Delimiter delimiter,
+                                      function_ref<ParseResult()> parseElt,
+                                      StringRef contextMessage) override {
+    return parser.parseCommaSeparatedList(delimiter, parseElt, contextMessage);
+  }
+
   //===--------------------------------------------------------------------===//
   // Attribute Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse an arbitrary attribute of a given type and return it in result.
   ParseResult parseAttribute(Attribute &result, Type type) override {
     result = parser.parseAttribute(type);
     return success(static_cast<bool>(result));
public:
   /// Parse zero or more SSA comma-separated operand or region arguments with
   ///  optional surrounding delimiter and required operand count.
   ParseResult
   parseOperandOrRegionArgList(SmallVectorImpl<OperandType> &result,
                               bool isOperandList, int requiredOperandCount = -1,
                               Delimiter delimiter = Delimiter::None) {
     auto startLoc = parser.getToken().getLoc();
 
-    // Handle delimiters.
-    switch (delimiter) {
-    case Delimiter::None:
-      // Don't check for the absence of a delimiter if the number of operands
-      // is unknown (and hence the operand list could be empty).
-      if (requiredOperandCount == -1)
-        break;
-      // Token already matches an identifier and so can't be a delimiter.
-      if (parser.getToken().is(Token::percent_identifier))
-        break;
-      // Test against known delimiters.
+    // The no-delimiter case has some special handling for better diagnostics.
+    if (delimiter == Delimiter::None) {
+      // parseCommaSeparatedList doesn't handle the missing case for "none",
+      // so we handle it custom here.
+      if (parser.getToken().isNot(Token::percent_identifier)) {
+        // If we didn't require any operands or required exactly zero (weird)
+        // then this is success.
+        if (requiredOperandCount == -1 || requiredOperandCount == 0)
+          return success();
+
+        // Otherwise, try to produce a nice error message.
         if (parser.getToken().is(Token::l_paren) ||
             parser.getToken().is(Token::l_square))
           return emitError(startLoc, "unexpected delimiter");
         return emitError(startLoc, "invalid operand");
-    case Delimiter::OptionalParen:
-      if (parser.getToken().isNot(Token::l_paren))
-        return success();
-      LLVM_FALLTHROUGH;
-    case Delimiter::Paren:
-      if (parser.parseToken(Token::l_paren, "expected '(' in operand list"))
-        return failure();
-      break;
-    case Delimiter::OptionalSquare:
-      if (parser.getToken().isNot(Token::l_square))
-        return success();
-      LLVM_FALLTHROUGH;
-    case Delimiter::Square:
-      if (parser.parseToken(Token::l_square, "expected '[' in operand list"))
-        return failure();
-      break;
+      }
     }
 
-    // Check for zero operands.
-    if (parser.getToken().is(Token::percent_identifier)) {
-      do {
-        OperandType operandOrArg;
-        if (isOperandList ? parseOperand(operandOrArg)
-                          : parseRegionArgument(operandOrArg))
-          return failure();
-        result.push_back(operandOrArg);
-      } while (parser.consumeIf(Token::comma));
-    }
+    auto parseOneOperand = [&]() -> ParseResult {
+      return success();
+    };
 
-    // Handle delimiters.   If we reach here, the optional delimiters were
-    // present, so we need to parse their closing one.
-    switch (delimiter) {
-    case Delimiter::None:
-      break;
-    case Delimiter::OptionalParen:
-    case Delimiter::Paren:
-      if (parser.parseToken(Token::r_paren, "expected ')' in operand list"))
-        return failure();
-      break;
-    case Delimiter::OptionalSquare:
-    case Delimiter::Square:
-      if (parser.parseToken(Token::r_square, "expected ']' in operand list"))
-        return failure();
-      break;
-    }
+    if (parseCommaSeparatedList(delimiter, parseOneOperand, " in operand list"))
 
+    // Check that we got the expected # of elements.
     if (requiredOperandCount != -1 &&
         result.size() != static_cast<size_t>(requiredOperandCount))
       return emitError(startLoc, "expected ")
              << requiredOperandCount << " operands";
     return success();
   }
 
   /// Parse zero or more trailing SSA comma-separated trailing operand

mlir/lib/Parser/TypeParser.cpp

ParseResult Parser::parseStridedLayout(int64_t &offset,
   offset = maybeOffset ? static_cast<int64_t>(maybeOffset.getValue())
                        : MemRefType::getDynamicStrideOrOffset();
   consumeToken();
 
   if (!consumeIf(Token::comma))
     return emitError("expected comma after offset value");
 
   // Parse stride list.
-  if (!consumeIf(Token::kw_strides))
-    return emitError("expected `strides` keyword after offset specification");
-  if (!consumeIf(Token::colon))
-    return emitError("expected colon after `strides` keyword");
-  if (failed(parseStrideList(strides)))
-    return emitError("invalid braces-enclosed stride list");
-  if (llvm::any_of(strides, [](int64_t st) { return st == 0; }))
-    return emitError("invalid memref stride");
+  if (parseToken(Token::kw_strides,
+                 "expected `strides` keyword after offset specification") ||
 
+      parseToken(Token::colon, "expected colon after `strides` keyword") ||
+      parseStrideList(strides))
+    return failure();
   return success();
 }
 
 /// Parse a memref type.
 ///
 ///   memref-type ::= ranked-memref-type | unranked-memref-type
 ///
 ///   ranked-memref-type ::= `memref` `<` dimension-list-ranked type
ParseResult Parser::parseXInDimensionList() {
   consumeToken(Token::bare_identifier);
 
   return success();
 }
 
 // Parse a comma-separated list of dimensions, possibly empty:
 //   stride-list ::= `[` (dimension (`,` dimension)*)? `]`
 ParseResult Parser::parseStrideList(SmallVectorImpl<int64_t> &dimensions) {
-  if (!consumeIf(Token::l_square))
-    return failure();
-  // Empty list early exit.
-  if (consumeIf(Token::r_square))
-    return success();
-  while (true) {
+  return parseCommaSeparatedList(
+      Delimiter::Square,
+      [&]() -> ParseResult {
         if (consumeIf(Token::question)) {
           dimensions.push_back(MemRefType::getDynamicStrideOrOffset());
         } else {
           // This must be an integer value.
           int64_t val;
           if (getToken().getSpelling().getAsInteger(10, val))
-        return emitError("invalid integer value: ") << getToken().getSpelling();
+            return emitError("invalid integer value: ")
+                   << getToken().getSpelling();
           // Make sure it is not the one value for `?`.
           if (ShapedType::isDynamic(val))
             return emitError("invalid integer value: ")
                    << getToken().getSpelling()
                    << ", use `?` to specify a dynamic dimension";
+
+          if (val == 0)
+            return emitError("invalid memref stride");
+
           dimensions.push_back(val);
           consumeToken(Token::integer);
         }
-    if (!consumeIf(Token::comma))
-      break;
-  }
-  if (!consumeIf(Token::r_square))
-    return failure();
         return success();
+      },
+      " in stride list");
 }

mlir/test/IR/invalid-affinemap.mlir

 
 // -----
 #hello_world = affine_map<(i, j) [s0] -> ((s0 + i, j)> // expected-error {{expected ')'}}
 
 // -----
 #hello_world = affine_map<(i, j) [s0] -> (((s0 + (i + j) + 5), j)> // expected-error {{expected ')'}}
 
 // -----
-#hello_world = affine_map<(i, j) [s0] -> i + s0, j)> // expected-error {{expected '(' at start of affine map range}}
+ 
+// expected-error @+1 {{expected '(' in affine map range}}
+#hello_world = affine_map<(i, j) [s0] -> i + s0, j)>
 
 // -----
 #hello_world = affine_map<(i, j) [s0] -> (x)> // expected-error {{use of undeclared identifier}}
 
 // -----
 #hello_world = affine_map<(i, j, i) [s0] -> (i)> // expected-error {{redefinition of identifier 'i'}}
 
 // -----
 #hello_world = affine_map<(i, j) [s0, s1, s0] -> (i)> // expected-error {{redefinition of identifier 's0'}}
 
 // -----
 #hello_world = affine_map<(i, j) [i, s0] -> (j)> // expected-error {{redefinition of identifier 'i'}}
 
 // -----
 #hello_world = affine_map<(i, j) [s0, s1] -> (+i, j)> // expected-error {{missing left operand of binary op}}
 
 // -----
+
+
 #hello_world = affine_map<(i, j) [s0, s1] -> (i, *j)> // expected-error {{missing left operand of binary op}}
 
 // -----
 #hello_world = affine_map<(i, j) [s0, s1] -> (floordiv i 2, j)> // expected-error {{missing left operand of binary op}}
 
 // -----
 #hello_world = affine_map<(i, j) [s0, s1] -> (ceildiv i 2, j)> // expected-error {{missing left operand of binary op}}
 

 
 // -----
 #hello_world = affine_map<(i, j) [s0, s1] -> (i, i*2 ceildiv j*5)> // expected-error {{non-affine expression: right operand of ceildiv has to be either a constant or symbolic}}
 
 // -----
 #hello_world = affine_map<(i, j) [s0, s1] -> (i, i mod (2+i))> // expected-error {{non-affine expression: right operand of mod has to be either a constant or symbolic}}
 
 // -----
-#hello_world = affine_map<(i, j) [s0, s1] -> (-1*i j, j)> // expected-error {{expected ',' or ')'}}
+// expected-error @+1 {{expected ')' in affine map range}}
+#hello_world = affine_map<(i, j) [s0, s1] -> (-1*i j, j)>
 
 // -----
 #hello_world = affine_map<(i, j) -> (i, 3*d0 + )> // expected-error {{use of undeclared identifier}}
 
 // TODO: Add more tests; coverage of error messages emitted not complete
 
 // -----
 #ABC = affine_map<(i,j) -> (i+j)>
 #ABC = affine_map<(i,j) -> (i+j)>  // expected-error {{redefinition of attribute alias id 'ABC'}}

mlir/test/IR/invalid.mlir

 func @memref_stride_missing_strides(memref<42x42xi8, offset: 0, [?, ?]>) // expected-error {{expected `strides` keyword after offset specification}}
 
 // -----
 
 func @memref_stride_missing_colon_2(memref<42x42xi8, offset: 0, strides [?, ?]>) // expected-error {{expected colon after `strides` keyword}}
 
 // -----
 
-func @memref_stride_invalid_strides(memref<42x42xi8, offset: 0, strides: ()>) // expected-error {{invalid braces-enclosed stride list}}
+// expected-error @+1 {{expected '['}}
+func @memref_stride_invalid_strides(memref<42x42xi8, offset: 0, strides: ()>)
 
 // -----
 
 func @memref_zero_stride(memref<42x42xi8, offset: ?, strides: [0, ?]>) // expected-error {{invalid memref stride}}
 
 // -----
 
 func @tensor_encoding_mismatch(%arg0: tensor<8xi32, "enc">) -> (tensor<8xi32>) { // expected-note {{prior use here}}

 func @invalid_bound_map(%N : i32) {
   affine.for %i = 1 to affine_map<(i)->(j)>(%N) { //expected-error {{use of undeclared identifier}}
   }
   return
 }
 
 // -----
 
-#set0 = affine_set<(i)[N, M] : )i >= 0)> // expected-error {{expected '(' at start of integer set constraint list}}
+// expected-error @+1 {{expected '(' in integer set constraint list}}
+#set0 = affine_set<(i)[N, M] : )i >= 0)>
 
 // -----
 #set0 = affine_set<(i)[N] : (i >= 0, N - i >= 0)>
 
 func @invalid_if_operands1(%N : index) {
   affine.for %i = 1 to 10 {
     affine.if #set0(%i) {
     // expected-error@-1 {{symbol operand count and integer set symbol count must match}}