[mlir] Refactoring dialect and test code to use parseCommaSeparatedList

[jakubtuchol]

Summary

Issue #55173

[lattner]

mlir/include/mlir/IR/OpImplementation.h

939

I'd recommend passing the lambda inline instead of naming it. I also learned a trick where "emplace_back" will add a new empty entry to a vector and return a reference to it. This should allow you to turn this into:

return parseCommaSeparatedList([&]() {
  return parseType(result.emplace_back());
});

mlir/lib/Dialect/Affine/IR/AffineOps.cpp

3490

These parser hooks support chaining with ||, plz give this a try:

if (parser.parseCommaSeparatedList(parseOperands)) ||
    parser.parseRParen())
  return failure();

mlir/lib/Dialect/DLTI/DLTI.cpp

295

Please use the same things above - you can move this inline, and use the emplace_back inline.

[jakubtuchol]

Addressing lattner's review

[lattner]

Nice, thank you! A couple more suggestions above.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

557–559

You can simplify this one to "return parser.parseOperand(...);"

3093

I /think/ you can use "return parser.emitError(...)" because the result converts to failure already.

3099

here and below too (if I'm right)

[rriddle]

LG, much nicer! Thanks for tackling this.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

2891–2892

Can you move the lambda out of line here? It's kind of difficult to read here.

3086

Same here.

mlir/lib/Dialect/PDL/IR/PDL.cpp

163

Same here.

mlir/lib/Dialect/PDLInterp/IR/PDLInterp.cpp

87

Same here.

[jakubtuchol]

Addressing lattner's rriddle's review

[jakubtuchol]

Fixing clang-fmt issues

[lattner]

nice, congrats on your first patch!

mlir/include/mlir/IR/OpImplementation.h

public:
   template <typename TypeT>
   std::enable_if_t<!detect_type_has_parse_method<TypeT>::value, ParseResult>
   parseCustomTypeWithFallback(TypeT &result) {
     return parseType(result);
   }
 
   /// Parse a type list.
   ParseResult parseTypeList(SmallVectorImpl<Type> &result) {
-    do {
+    auto parse = [&]() {
       Type type;
       if (parseType(type))
         return failure();
       result.push_back(type);
-    } while (succeeded(parseOptionalComma()));
       return success();
+    };
+    return parseCommaSeparatedList(parse);
   }
 
   /// Parse an arrow followed by a type list.
   virtual ParseResult parseArrowTypeList(SmallVectorImpl<Type> &result) = 0;
 
   /// Parse an optional arrow followed by a type list.
   virtual ParseResult
   parseOptionalArrowTypeList(SmallVectorImpl<Type> &result) = 0;

mlir/lib/Dialect/Affine/IR/AffineOps.cpp

if (succeeded(parser.parseOptionalRParen())) {
     return success();
   }
 
   SmallVector<AffineExpr> flatExprs;
   SmallVector<SmallVector<OpAsmParser::UnresolvedOperand>> flatDimOperands;
   SmallVector<SmallVector<OpAsmParser::UnresolvedOperand>> flatSymOperands;
   SmallVector<int32_t> numMapsPerGroup;
   SmallVector<OpAsmParser::UnresolvedOperand> mapOperands;
-  do {
+  auto parseOperands = [&]() {
     if (succeeded(parser.parseOptionalKeyword(
             kind == MinMaxKind::Min ? "min" : "max"))) {
       mapOperands.clear();
       AffineMapAttr map;
       if (failed(parser.parseAffineMapOfSSAIds(mapOperands, map, tmpAttrName,
                                                result.attributes,
                                                OpAsmParser::Delimiter::Paren)))
         return failure();
if (succeeded(parser.parseOptionalKeyword(
       numMapsPerGroup.push_back(map.getValue().getNumResults());
     } else {
       if (failed(parser.parseAffineExprOfSSAIds(flatDimOperands.emplace_back(),
                                                 flatSymOperands.emplace_back(),
                                                 flatExprs.emplace_back())))
         return failure();
       numMapsPerGroup.push_back(1);
     }
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseOperands)))
+    return failure();
 
   if (failed(parser.parseRParen()))
     return failure();
 
   unsigned totalNumDims = 0;
   unsigned totalNumSyms = 0;
   for (unsigned i = 0, e = flatExprs.size(); i < e; ++i) {
     unsigned numDims = flatDimOperands[i].size();
     unsigned numSyms = flatSymOperands[i].size();
     flatExprs[i] = flatExprs[i]

mlir/lib/Dialect/DLTI/DLTI.cpp

DataLayoutSpecAttr DataLayoutSpecAttr::parse(AsmParser &parser) {
   if (failed(parser.parseLess()))
     return {};
 
   // Empty spec.
   if (succeeded(parser.parseOptionalGreater()))
     return get(parser.getContext(), {});
 
   SmallVector<DataLayoutEntryInterface> entries;
-  do {
+  auto parseEntries = [&]() {
     entries.emplace_back();
     if (failed(parser.parseAttribute(entries.back())))
-      return {};
-  } while (succeeded(parser.parseOptionalComma()));
+      return failure();
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseEntries)))
 
   if (failed(parser.parseGreater()))
     return {};
   return getChecked([&] { return parser.emitError(parser.getNameLoc()); },
                     parser.getContext(), entries);
 }
 
 void DataLayoutSpecAttr::print(AsmPrinter &os) const {

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

void GEPOp::build(OpBuilder &builder, OperationState &result, Type resultType,
   result.addOperands(remainingIndices);
 }
 
 static ParseResult
 parseGEPIndices(OpAsmParser &parser,
                 SmallVectorImpl<OpAsmParser::UnresolvedOperand> &indices,
                 DenseIntElementsAttr &structIndices) {
   SmallVector<int32_t> constantIndices;
-  do {
+  auto parseIndices = [&]() {
     int32_t constantIndex;
     OptionalParseResult parsedInteger =
         parser.parseOptionalInteger(constantIndex);
     if (parsedInteger.hasValue()) {
       if (failed(parsedInteger.getValue()))
         return failure();
       constantIndices.push_back(constantIndex);
-      continue;
+      return success();
     }
 
     constantIndices.push_back(LLVM::GEPOp::kDynamicIndex);
     if (failed(parser.parseOperand(indices.emplace_back())))
       return failure();
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  parser.parseCommaSeparatedList(parseIndices);
 
   structIndices = parser.getBuilder().getI32TensorAttr(constantIndices);
   return success();
 }
 
 static void printGEPIndices(OpAsmPrinter &printer, LLVM::GEPOp gepOp,
                             OperandRange indices,
                             DenseIntElementsAttr structIndices) {

 }
 
 Attribute FMFAttr::parse(AsmParser &parser, Type type) {
   if (failed(parser.parseLess()))
     return {};
 
   FastmathFlags flags = {};
   if (failed(parser.parseOptionalGreater())) {
-    do {
+    auto parseFlags = [&]() {
       StringRef elemName;
       if (failed(parser.parseKeyword(&elemName)))
-        return {};
+        return failure();
 
       auto elem = symbolizeFastmathFlags(elemName);
       if (!elem) {
         parser.emitError(parser.getNameLoc(), "Unknown fastmath flag: ")
             << elemName;
-        return {};
+        return failure();
       }
 
       flags = flags | *elem;
-    } while (succeeded(parser.parseOptionalComma()));
+      return success();
+    };
+    if (failed(parser.parseCommaSeparatedList(parseFlags)))
+      return {};
 
     if (failed(parser.parseGreater()))
       return {};
   }
 
   return FMFAttr::get(parser.getContext(), flags);
 }
 
 void LinkageAttr::print(AsmPrinter &printer) const {
   printer << "<";
   if (static_cast<uint64_t>(getLinkage()) <= getMaxEnumValForLinkage())

 }
 
 Attribute LoopOptionsAttr::parse(AsmParser &parser, Type type) {
   if (failed(parser.parseLess()))
     return {};
 
   SmallVector<std::pair<LoopOptionCase, int64_t>> options;
   llvm::SmallDenseSet<LoopOptionCase> seenOptions;
-  do {
+  auto parseLoopOption = [&]() {
     StringRef optionName;
     if (parser.parseKeyword(&optionName))
-      return {};
+      return failure();
 
     auto option = symbolizeLoopOptionCase(optionName);
     if (!option) {
       parser.emitError(parser.getNameLoc(), "unknown loop option: ")
           << optionName;
-      return {};
+      return failure();
     }
     if (!seenOptions.insert(*option).second) {
       parser.emitError(parser.getNameLoc(), "loop option present twice");
-      return {};
+      return failure();
     }
     if (failed(parser.parseEqual()))
-      return {};
+      return failure();
 
     int64_t value;
     switch (*option) {
     case LoopOptionCase::disable_licm:
     case LoopOptionCase::disable_unroll:
     case LoopOptionCase::disable_pipeline:
       if (succeeded(parser.parseOptionalKeyword("true")))
         value = 1;
       else if (succeeded(parser.parseOptionalKeyword("false")))
         value = 0;
       else {
         parser.emitError(parser.getNameLoc(),
                          "expected boolean value 'true' or 'false'");
-        return {};
+        return failure();
       }
       break;
     case LoopOptionCase::interleave_count:
     case LoopOptionCase::pipeline_initiation_interval:
       if (failed(parser.parseInteger(value))) {
         parser.emitError(parser.getNameLoc(), "expected integer value");
-        return {};
+        return failure();
       }
       break;
     }
     options.push_back(std::make_pair(*option, value));
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseLoopOption)))
+    return {};
+
   if (failed(parser.parseGreater()))
     return {};
 
   llvm::sort(options, llvm::less_first());
   return get(parser.getContext(), options);
 }

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp

parseOperandList(OpAsmParser &parser, StringRef keyword,
 
   if (failed(parser.parseLParen()))
     return failure();
 
   // Exit early if the list is empty.
   if (succeeded(parser.parseOptionalRParen()))
     return success();
 
-  do {
+  auto parseOperands = [&]() {
     OpAsmParser::UnresolvedOperand arg;
     Type type;
 
     if (parser.parseOperand(arg, /*allowResultNumber=*/false) ||
         parser.parseColonType(type))
       return failure();
 
     args.push_back(arg);
     argTypes.push_back(type);
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseOperands)))
+    return failure();
 
   if (failed(parser.parseRParen()))
     return failure();
 
   return parser.resolveOperands(args, argTypes, parser.getCurrentLocation(),
                                 result.operands);
 }
 

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

 /// linear ::= `linear` `(` linear-list `)`
 /// linear-list := linear-val | linear-val linear-list
 /// linear-val := ssa-id-and-type `=` ssa-id-and-type
 static ParseResult
 parseLinearClause(OpAsmParser &parser,
                   SmallVectorImpl<OpAsmParser::UnresolvedOperand> &vars,
                   SmallVectorImpl<Type> &types,
                   SmallVectorImpl<OpAsmParser::UnresolvedOperand> &stepVars) {
-  do {
+  return parser.parseCommaSeparatedList([&]() -> ParseResult {
     OpAsmParser::UnresolvedOperand var;
     Type type;
     OpAsmParser::UnresolvedOperand stepVar;
     if (parser.parseOperand(var) || parser.parseEqual() ||
         parser.parseOperand(stepVar) || parser.parseColonType(type))
       return failure();
 
     vars.push_back(var);
     types.push_back(type);
     stepVars.push_back(stepVar);
-  } while (succeeded(parser.parseOptionalComma()));
     return success();
+  });
 }
 
 /// Print Linear Clause
 static void printLinearClause(OpAsmPrinter &p, Operation *op,
                               ValueRange linearVars, TypeRange linearVarTypes,
                               ValueRange linearStepVars) {
   size_t linearVarsSize = linearVars.size();
   for (unsigned i = 0; i < linearVarsSize; ++i) {

 ///                        | reduction-entry-list `,` reduction-entry
 /// reduction-entry ::= symbol-ref `->` ssa-id `:` type
 static ParseResult
 parseReductionVarList(OpAsmParser &parser,
                       SmallVectorImpl<OpAsmParser::UnresolvedOperand> &operands,
                       SmallVectorImpl<Type> &types,
                       ArrayAttr &redcuctionSymbols) {
   SmallVector<SymbolRefAttr> reductionVec;
-  do {
+  auto parseReductionList = [&]() {
     if (parser.parseAttribute(reductionVec.emplace_back()) ||
         parser.parseArrow() || parser.parseOperand(operands.emplace_back()) ||
         parser.parseColonType(types.emplace_back()))
       return failure();
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseReductionList)))
+    return failure();
   SmallVector<Attribute> reductions(reductionVec.begin(), reductionVec.end());
   redcuctionSymbols = ArrayAttr::get(parser.getContext(), reductions);
   return success();
 }
 
 /// Print Reduction clause
 static void printReductionVarList(OpAsmPrinter &p, Operation *op,
                                   OperandRange reductionVars,

mlir/lib/Dialect/PDL/IR/PDL.cpp

 
 static ParseResult parseOperationOpAttributes(
     OpAsmParser &p,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &attrOperands,
     ArrayAttr &attrNamesAttr) {
   Builder &builder = p.getBuilder();
   SmallVector<Attribute, 4> attrNames;
   if (succeeded(p.parseOptionalLBrace())) {
-    do {
+    auto parseOperands = [&]() {
       StringAttr nameAttr;
       OpAsmParser::UnresolvedOperand operand;
       if (p.parseAttribute(nameAttr) || p.parseEqual() ||
           p.parseOperand(operand))
         return failure();
       attrNames.push_back(nameAttr);
       attrOperands.push_back(operand);
-    } while (succeeded(p.parseOptionalComma()));
+      return success();
+    };
+    if (failed(p.parseCommaSeparatedList(parseOperands)))
+      return failure();
     if (p.parseRBrace())
       return failure();
   }
   attrNamesAttr = builder.getArrayAttr(attrNames);
   return success();
 }
 
 static void printOperationOpAttributes(OpAsmPrinter &p, OperationOp op,
                                        OperandRange attrArgs,

mlir/lib/Dialect/PDLInterp/IR/PDLInterp.cpp

 
 static ParseResult parseCreateOperationOpAttributes(
     OpAsmParser &p,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &attrOperands,
     ArrayAttr &attrNamesAttr) {
   Builder &builder = p.getBuilder();
   SmallVector<Attribute, 4> attrNames;
   if (succeeded(p.parseOptionalLBrace())) {
-    do {
+    auto parseOperands = [&]() {
       StringAttr nameAttr;
       OpAsmParser::UnresolvedOperand operand;
       if (p.parseAttribute(nameAttr) || p.parseEqual() ||
           p.parseOperand(operand))
         return failure();
       attrNames.push_back(nameAttr);
       attrOperands.push_back(operand);
-    } while (succeeded(p.parseOptionalComma()));
+      return success();
+    };
+    if (failed(p.parseCommaSeparatedList(parseOperands)))
+      return failure();
+
     if (p.parseRBrace())
       return failure();
   }
   attrNamesAttr = builder.getArrayAttr(attrNames);
   return success();
 }
 
 static void printCreateOperationOpAttributes(OpAsmPrinter &p,
                                              CreateOperationOp op,

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

static ParseResult parseStructMemberDecorations(
   if (succeeded(parser.parseOptionalRSquare()))
     return success();
 
   // If there was an offset, make sure to parse the comma.
   if (offsetParseResult.hasValue() && parser.parseComma())
     return failure();
 
   // Check for spirv::Decorations.
-  do {
+  auto parseDecorations = [&]() {
     auto memberDecoration = parseAndVerify<spirv::Decoration>(dialect, parser);
     if (!memberDecoration)
       return failure();
 
     // Parse member decoration value if it exists.
     if (succeeded(parser.parseOptionalEqual())) {
       auto memberDecorationValue =
           parseAndVerifyInteger<uint32_t>(dialect, parser);
 
       if (!memberDecorationValue)
         return failure();
 
       memberDecorationInfo.emplace_back(
           static_cast<uint32_t>(memberTypes.size() - 1), 1,
           memberDecoration.getValue(), memberDecorationValue.getValue());
     } else {
       memberDecorationInfo.emplace_back(
           static_cast<uint32_t>(memberTypes.size() - 1), 0,
           memberDecoration.getValue(), 0);
     }
-
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseDecorations)))
+    return failure();
 
   return parser.parseRSquare();
 }
 
 // struct-member-decoration ::= integer-literal? spirv-decoration*
 // struct-type ::=
 //             `!spv.struct<` (id `,`)?
 //                          `(`
if (parser.parseLSquare())
     return failure();
 
   // Special case for empty list.
   if (succeeded(parser.parseOptionalRSquare()))
     return success();
 
   // Keep parsing the keyword and an optional comma following it. If the comma
   // is successfully parsed, then we have more keywords to parse.
-  do {
+  auto parseKeyword = [&]() {
     auto loc = parser.getCurrentLocation();
     StringRef keyword;
     if (parser.parseKeyword(&keyword) || failed(processKeyword(loc, keyword)))
       return failure();
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseKeyword)))
+    return failure();
 
   if (parser.parseRSquare())
     return failure();
 
   return success();
 }
 
 /// Parses a spirv::InterfaceVarABIAttr.

mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp

Attribute parseAttribute(DialectAsmParser &parser, Type type) const override {
     bool ok =
         succeeded(parser.parseKeyword("spec")) && succeeded(parser.parseLess());
     (void)ok;
     assert(ok);
     if (succeeded(parser.parseOptionalGreater()))
       return CustomDataLayoutSpec::get(parser.getContext(), {});
 
     SmallVector<DataLayoutEntryInterface> entries;
-    do {
+    auto parseEntries = [&]() {
       entries.emplace_back();
       ok = succeeded(parser.parseAttribute(entries.back()));
       assert(ok);
-    } while (succeeded(parser.parseOptionalComma()));
+      return success();
+    };
+    parser.parseCommaSeparatedList(parseEntries);
     ok = succeeded(parser.parseGreater());
     assert(ok);
     return CustomDataLayoutSpec::get(parser.getContext(), entries);
   }
 
   void printType(Type type, DialectAsmPrinter &printer) const override {
     if (type.isa<SingleQueryType>())
       printer << "single_query";