[MLIR] correct return type of parse() functions

[tim-hoffman]

The parseX() functions that are defined to support custom<X> in assemblyFormat should return ParseResult rather than LogicalResult. The ParseResult type is necessary due to tablegen generating code that expects this type within an Op parseX() function.

[llvmbot]

@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-gpu

Author: Timothy Hoffman (tim-hoffman)

Changes

see explanation here: #120179 (comment)

---

Full diff: https://github.com/llvm/llvm-project/pull/120180.diff

3 Files Affected:

• (modified) mlir/lib/Dialect/GPU/IR/GPUDialect.cpp (+1-1)
• (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp (+2-2)
• (modified) mlir/test/lib/Dialect/Test/TestTypes.cpp (+4-4)

diff --git a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
index ed2d81ee65eb4a..6ec327402a4a85 100644
--- a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
+++ b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
@@ -2114,7 +2114,7 @@ LogicalResult ObjectAttr::verify(function_ref<InFlightDiagnostic()> emitError,
 }
 
 namespace {
-LogicalResult parseObject(AsmParser &odsParser, CompilationTarget &format,
+ParseResult parseObject(AsmParser &odsParser, CompilationTarget &format,
                           StringAttr &object) {
   std::optional<CompilationTarget> formatResult;
   StringRef enumKeyword;
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
index ee4e344674a67e..441c419379cafd 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
@@ -27,7 +27,7 @@ using namespace mlir::LLVM;
 /// Parses DWARF expression arguments with respect to the DWARF operation
 /// opcode. Some DWARF expression operations have a specific number of operands
 /// and may appear in a textual form.
-static LogicalResult parseExpressionArg(AsmParser &parser, uint64_t opcode,
+static ParseResult parseExpressionArg(AsmParser &parser, uint64_t opcode,
                                         SmallVector<uint64_t> &args);
 
 /// Prints DWARF expression arguments with respect to the specific DWARF
@@ -144,7 +144,7 @@ DIExpressionAttr DIExpressionAttr::get(MLIRContext *context) {
   return get(context, ArrayRef<DIExpressionElemAttr>({}));
 }
 
-LogicalResult parseExpressionArg(AsmParser &parser, uint64_t opcode,
+ParseResult parseExpressionArg(AsmParser &parser, uint64_t opcode,
                                  SmallVector<uint64_t> &args) {
   auto operandParser = [&]() -> LogicalResult {
     uint64_t operand = 0;
diff --git a/mlir/test/lib/Dialect/Test/TestTypes.cpp b/mlir/test/lib/Dialect/Test/TestTypes.cpp
index 1593b6d7d7534b..92af5613b38391 100644
--- a/mlir/test/lib/Dialect/Test/TestTypes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestTypes.cpp
@@ -90,13 +90,13 @@ static llvm::hash_code test::hash_value(const FieldInfo &fi) { // NOLINT
 // TestCustomType
 //===----------------------------------------------------------------------===//
 
-static LogicalResult parseCustomTypeA(AsmParser &parser, int &aResult) {
+static ParseResult parseCustomTypeA(AsmParser &parser, int &aResult) {
   return parser.parseInteger(aResult);
 }
 
 static void printCustomTypeA(AsmPrinter &printer, int a) { printer << a; }
 
-static LogicalResult parseCustomTypeB(AsmParser &parser, int a,
+static ParseResult parseCustomTypeB(AsmParser &parser, int a,
                                       std::optional<int> &bResult) {
   if (a < 0)
     return success();
@@ -116,7 +116,7 @@ static void printCustomTypeB(AsmPrinter &printer, int a, std::optional<int> b) {
   printer << *b;
 }
 
-static LogicalResult parseFooString(AsmParser &parser, std::string &foo) {
+static ParseResult parseFooString(AsmParser &parser, std::string &foo) {
   std::string result;
   if (parser.parseString(&result))
     return failure();
@@ -128,7 +128,7 @@ static void printFooString(AsmPrinter &printer, StringRef foo) {
   printer << '"' << foo << '"';
 }
 
-static LogicalResult parseBarString(AsmParser &parser, StringRef foo) {
+static ParseResult parseBarString(AsmParser &parser, StringRef foo) {
   return parser.parseKeyword(foo);
 }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Dinistro]

Please ensure that you change the PR title to use the proper formatting rules defined for llvm and expand on the description, as this will be turned into a commit message.

Apart from this, this LGTM! Thanks for the fix 🙂

[tim-hoffman]

Here is an example of code generated by tablegen that will cause a compiler error if the parseX() function returns LogicalResult:

    auto odsResult = parseInferredArrayType(parser, elementsTypes, elementsOperands, resultRawTypes[0]);
    if (odsResult) return ::mlir::failure();

See also parsePrettyLLVMType() in LLVMTypes.h, parseSingleBlockRegion() in IRDL.cpp, parseDynamicIndexList() in ViewLikeInterface.cpp, etc.

[tim-hoffman]

@Dinistro Please let me know if any additional changes are needed. Thanks!
Also, will need someone with write access to merge once it's all good.

[Dinistro]

Looks fine. @krzysz00 thanks for merging this.