[mlir] Refactoring dialect and test code to use parseCommaSeparatedList

[jakubtuchol]

Summary

Issue #55173

[lattner]

mlir/include/mlir/IR/OpImplementation.h

934

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

3485–3487

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

291

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

555–556

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

3074

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

3080

here and below too (if I'm right)

[rriddle]

LG, much nicer! Thanks for tackling this.

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

2881–2885

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

3067–3069

Same here.

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

159–161

Same here.

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

82–84

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 {
-      Type type;
-      if (parseType(type))
-        return failure();
-      result.push_back(type);
-    } while (succeeded(parseOptionalComma()));
-    return success();
+    return parseCommaSeparatedList(
+        [&]() { return parseType(result.emplace_back()); });
   }
 
   /// 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()));
-
-  if (failed(parser.parseRParen()))
+    return success();
+  };
+  if (parser.parseCommaSeparatedList(parseOperands) || 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]
ParseResult AffineParallelOp::parse(OpAsmParser &parser,
   }
 
   // Parse optional clause of the form: `reduce ("addf", "maxf")`, where the
   // quoted strings are a member of the enum AtomicRMWKind.
   SmallVector<Attribute, 4> reductions;
   if (succeeded(parser.parseOptionalKeyword("reduce"))) {
     if (parser.parseLParen())
       return failure();
-    do {
+    auto parseAttributes = [&]() -> ParseResult {
       // Parse a single quoted string via the attribute parsing, and then
       // verify it is a member of the enum and convert to it's integer
       // representation.
       StringAttr attrVal;
       NamedAttrList attrStorage;
       auto loc = parser.getCurrentLocation();
       if (parser.parseAttribute(attrVal, builder.getNoneType(), "reduce",
                                 attrStorage))
         return failure();
       llvm::Optional<arith::AtomicRMWKind> reduction =
           arith::symbolizeAtomicRMWKind(attrVal.getValue());
       if (!reduction)
         return parser.emitError(loc, "invalid reduction value: ") << attrVal;
       reductions.push_back(builder.getI64IntegerAttr(
           static_cast<int64_t>(reduction.getValue())));
       // While we keep getting commas, keep parsing.
-    } while (succeeded(parser.parseOptionalComma()));
-    if (parser.parseRParen())
+      return success();
+    };
+    if (parser.parseCommaSeparatedList(parseAttributes) || parser.parseRParen())
       return failure();
   }
   result.addAttribute(AffineParallelOp::getReductionsAttrName(),
                       builder.getArrayAttr(reductions));
 
   // Parse return types of reductions (if any)
   if (parser.parseOptionalArrowTypeList(result.types))
     return failure();

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 {
-    entries.emplace_back();
-    if (failed(parser.parseAttribute(entries.back())))
-      return {};
-  } while (succeeded(parser.parseOptionalComma()));
+  if (parser.parseCommaSeparatedList(
+          [&]() { return parser.parseAttribute(entries.emplace_back()); }) ||
+      parser.parseGreater())
 
-  if (failed(parser.parseGreater()))
-    return {};
   return getChecked([&] { return parser.emitError(parser.getNameLoc()); },
                     parser.getContext(), entries);
 }
 
 void DataLayoutSpecAttr::print(AsmPrinter &os) const {
   os << DataLayoutSpecAttr::kAttrKeyword << "<";
   llvm::interleaveComma(getEntries(), os);
   os << ">";

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 {
+  parser.parseCommaSeparatedList([&]() -> ParseResult {
     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 parser.parseOperand(indices.emplace_back());
+  });
 
   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 = [&]() -> ParseResult {
       StringRef elemName;
       if (failed(parser.parseKeyword(&elemName)))
-        return {};
+        return failure();
 
       auto elem = symbolizeFastmathFlags(elemName);
-      if (!elem) {
-        parser.emitError(parser.getNameLoc(), "Unknown fastmath flag: ")
+      if (!elem)
+        return parser.emitError(parser.getNameLoc(), "Unknown fastmath flag: ")
                << elemName;
-        return {};
-      }
 
       flags = flags | *elem;
-    } while (succeeded(parser.parseOptionalComma()));
-
-    if (failed(parser.parseGreater()))
+      return success();
+    };
+    if (failed(parser.parseCommaSeparatedList(parseFlags)) ||
+        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 parseLoopOptions = [&]() -> ParseResult {
     StringRef optionName;
     if (parser.parseKeyword(&optionName))
-      return {};
+      return failure();
 
     auto option = symbolizeLoopOptionCase(optionName);
-    if (!option) {
-      parser.emitError(parser.getNameLoc(), "unknown loop option: ")
+    if (!option)
+      return parser.emitError(parser.getNameLoc(), "unknown loop option: ")
              << optionName;
-      return {};
-    }
-    if (!seenOptions.insert(*option).second) {
-      parser.emitError(parser.getNameLoc(), "loop option present twice");
-      return {};
-    }
+    if (!seenOptions.insert(*option).second)
+      return parser.emitError(parser.getNameLoc(), "loop option present twice");
     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(),
+        return parser.emitError(parser.getNameLoc(),
                                 "expected boolean value 'true' or 'false'");
-        return {};
       }
       break;
     case LoopOptionCase::interleave_count:
     case LoopOptionCase::pipeline_initiation_interval:
-      if (failed(parser.parseInteger(value))) {
-        parser.emitError(parser.getNameLoc(), "expected integer value");
-        return {};
-      }
+      if (failed(parser.parseInteger(value)))
+        return parser.emitError(parser.getNameLoc(), "expected integer value");
       break;
     }
     options.push_back(std::make_pair(*option, value));
-  } while (succeeded(parser.parseOptionalComma()));
-  if (failed(parser.parseGreater()))
+    return success();
+  };
+  if (parser.parseCommaSeparatedList(parseLoopOptions) || 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 {
+  if (failed(parser.parseCommaSeparatedList([&]() {
         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()));
-
-  if (failed(parser.parseRParen()))
+        return success();
+      })) ||
+      failed(parser.parseRParen()))
     return failure();
 
   return parser.resolveOperands(args, argTypes, parser.getCurrentLocation(),
                                 result.operands);
 }
 
 static void printOperandList(Operation::operand_range operands,
                              StringRef listName, OpAsmPrinter &printer) {

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

 /// ssa-id-and-type ::= ssa-id `:` type
 static ParseResult parseAllocateAndAllocator(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &operandsAllocate,
     SmallVectorImpl<Type> &typesAllocate,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &operandsAllocator,
     SmallVectorImpl<Type> &typesAllocator) {
 
-  return parser.parseCommaSeparatedList([&]() -> ParseResult {
+  return parser.parseCommaSeparatedList([&]() {
     OpAsmParser::UnresolvedOperand 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();

 /// 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([&]() {
     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 {
+  if (failed(parser.parseCommaSeparatedList([&]() {
         if (parser.parseAttribute(reductionVec.emplace_back()) ||
-        parser.parseArrow() || parser.parseOperand(operands.emplace_back()) ||
+            parser.parseArrow() ||
+            parser.parseOperand(operands.emplace_back()) ||
             parser.parseColonType(types.emplace_back()))
           return failure();
-  } while (succeeded(parser.parseOptionalComma()));
+        return success();
+      })))
+    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,

 static ParseResult parseSynchronizationHint(OpAsmParser &parser,
                                             IntegerAttr &hintAttr) {
   StringRef hintKeyword;
   int64_t hint = 0;
   if (succeeded(parser.parseOptionalKeyword("none"))) {
     hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), 0);
     return success();
   }
-  do {
+  auto parseKeyword = [&]() -> ParseResult {
     if (failed(parser.parseKeyword(&hintKeyword)))
       return failure();
     if (hintKeyword == "uncontended")
       hint |= 1;
     else if (hintKeyword == "contended")
       hint |= 2;
     else if (hintKeyword == "nonspeculative")
       hint |= 4;
     else if (hintKeyword == "speculative")
       hint |= 8;
     else
       return parser.emitError(parser.getCurrentLocation())
              << hintKeyword << " is not a valid hint";
-  } while (succeeded(parser.parseOptionalComma()));
+    return success();
+  };
+  if (parser.parseCommaSeparatedList(parseKeyword))
+    return failure();
   hintAttr = IntegerAttr::get(parser.getBuilder().getI64Type(), hint);
   return success();
 }
 
 /// Prints a Synchronization Hint clause
 static void printSynchronizationHint(OpAsmPrinter &p, Operation *op,
                                      IntegerAttr hintAttr) {
   int64_t hint = hintAttr.getInt();

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()));
-    if (p.parseRBrace())
+      return success();
+    };
+    if (p.parseCommaSeparatedList(parseOperands) || 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()));
-    if (p.parseRBrace())
+      return success();
+    };
+    if (p.parseCommaSeparatedList(parseOperands) || 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);
     }
+    return success();
+  };
+  if (failed(parser.parseCommaSeparatedList(parseDecorations)) ||
+      failed(parser.parseRSquare()))
+    return failure();
 
-  } while (succeeded(parser.parseOptionalComma()));
-
-  return parser.parseRSquare();
+  return success();
 }
 
 // struct-member-decoration ::= integer-literal? spirv-decoration*
 // struct-type ::=
 //             `!spv.struct<` (id `,`)?
 //                          `(`
 //                            (spirv-type (`[` struct-member-decoration `]`)?)*
 //                          `)>`
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 {
+  if (failed(parser.parseCommaSeparatedList([&]() {
         auto loc = parser.getCurrentLocation();
         StringRef keyword;
-    if (parser.parseKeyword(&keyword) || failed(processKeyword(loc, keyword)))
-      return failure();
-  } while (succeeded(parser.parseOptionalComma()));
-
-  if (parser.parseRSquare())
+        if (parser.parseKeyword(&keyword) ||
+            failed(processKeyword(loc, keyword)))
           return failure();
-
         return success();
+      })))
+    return failure();
+  return parser.parseRSquare();
 }
 
 /// Parses a spirv::InterfaceVarABIAttr.
 static Attribute parseInterfaceVarABIAttr(DialectAsmParser &parser) {
   if (parser.parseLess())
     return {};
 
   Builder &builder = parser.getBuilder();

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 {
+    parser.parseCommaSeparatedList([&]() {
       entries.emplace_back();
       ok = succeeded(parser.parseAttribute(entries.back()));
       assert(ok);
-    } while (succeeded(parser.parseOptionalComma()));
+      return success();
+    });
     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";