diff --git a/mlir/include/mlir/Support/LogicalResult.h b/mlir/include/mlir/Support/LogicalResult.h --- a/mlir/include/mlir/Support/LogicalResult.h +++ b/mlir/include/mlir/Support/LogicalResult.h @@ -109,7 +109,7 @@ /// swallowed up in boilerplate without this, so we provide this for narrow /// cases where it is important. /// -class ParseResult : public LogicalResult { +class LLVM_NODISCARD ParseResult : public LogicalResult { public: ParseResult(LogicalResult result = success()) : LogicalResult(result) {} diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp --- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp +++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp @@ -3382,7 +3382,7 @@ /// unique operands. Also populates `replacements with affine expressions of /// `kind` that can be used to update affine maps previously accepting a /// `operands` to accept `uniqueOperands` instead. -static void deduplicateAndResolveOperands( +static ParseResult deduplicateAndResolveOperands( OpAsmParser &parser, ArrayRef> operands, SmallVectorImpl &uniqueOperands, @@ -3393,7 +3393,8 @@ Type indexType = parser.getBuilder().getIndexType(); for (const auto &list : operands) { SmallVector valueOperands; - parser.resolveOperands(list, indexType, valueOperands); + if (parser.resolveOperands(list, indexType, valueOperands)) + return failure(); for (Value operand : valueOperands) { unsigned pos = std::distance(uniqueOperands.begin(), llvm::find(uniqueOperands, operand)); @@ -3405,6 +3406,7 @@ : getAffineSymbolExpr(pos, parser.getContext())); } } + return success(); } namespace { @@ -3502,10 +3504,11 @@ // Deduplicate map operands. SmallVector dimOperands, symOperands; SmallVector dimRplacements, symRepacements; - deduplicateAndResolveOperands(parser, flatDimOperands, dimOperands, - dimRplacements, AffineExprKind::DimId); - deduplicateAndResolveOperands(parser, flatSymOperands, symOperands, - symRepacements, AffineExprKind::SymbolId); + if (deduplicateAndResolveOperands(parser, flatDimOperands, dimOperands, + dimRplacements, AffineExprKind::DimId) || + deduplicateAndResolveOperands(parser, flatSymOperands, symOperands, + symRepacements, AffineExprKind::SymbolId)) + return failure(); result.operands.append(dimOperands.begin(), dimOperands.end()); result.operands.append(symOperands.begin(), symOperands.end()); diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp --- a/mlir/lib/Dialect/Async/IR/Async.cpp +++ b/mlir/lib/Dialect/Async/IR/Async.cpp @@ -210,17 +210,15 @@ // Parse the types of results returned from the async execute op. SmallVector resultTypes; - if (parser.parseOptionalArrowTypeList(resultTypes)) - return failure(); - - // Async execute first result is always a completion token. - parser.addTypeToList(tokenTy, result.types); - parser.addTypesToList(resultTypes, result.types); - - // Parse operation attributes. NamedAttrList attrs; - if (parser.parseOptionalAttrDictWithKeyword(attrs)) + if (parser.parseOptionalArrowTypeList(resultTypes) || + // Async execute first result is always a completion token. + parser.addTypeToList(tokenTy, result.types) || + parser.addTypesToList(resultTypes, result.types) || + // Parse operation attributes. + parser.parseOptionalAttrDictWithKeyword(attrs)) return failure(); + result.addAttributes(attrs); // Parse asynchronous region. diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp --- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp +++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp @@ -540,7 +540,8 @@ SmallVectorImpl &indices, DenseIntElementsAttr &structIndices) { SmallVector constantIndices; - parser.parseCommaSeparatedList([&]() -> ParseResult { + + auto idxParser = [&]() -> ParseResult { int32_t constantIndex; OptionalParseResult parsedInteger = parser.parseOptionalInteger(constantIndex); @@ -553,7 +554,9 @@ constantIndices.push_back(LLVM::GEPOp::kDynamicIndex); return parser.parseOperand(indices.emplace_back()); - }); + }; + if (parser.parseCommaSeparatedList(idxParser)) + return failure(); structIndices = parser.getBuilder().getI32TensorAttr(constantIndices); return success(); diff --git a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp --- a/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp +++ b/mlir/lib/Dialect/LLVMIR/IR/NVVMDialect.cpp @@ -306,8 +306,8 @@ } Type resultType; - parser.parseArrow(); - parser.parseType(resultType); + if (parser.parseArrow() || parser.parseType(resultType)) + return failure(); frags[3].elemtype = inferOperandMMAType(resultType, /*isAccum=*/true); std::array names{"multiplicandAPtxType", diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp --- a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp +++ b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp @@ -130,7 +130,8 @@ SmallVector inputsOperands, outputsOperands; - parser.parseOptionalAttrDict(result.attributes); + if (parser.parseOptionalAttrDict(result.attributes)) + return failure(); if (succeeded(parser.parseOptionalKeyword("ins"))) { if (parser.parseLParen()) diff --git a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp --- a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp +++ b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp @@ -118,11 +118,9 @@ OperationState &result) { StringRef sizesAttrName = TileOp::getSizesAttrName(result.name).getValue(); OpAsmParser::UnresolvedOperand targetOperand; - SMLoc opLoc; - parser.getCurrentLocation(&opLoc); - if (parser.parseOperand(targetOperand)) - return parser.emitError(opLoc, "expected 'target' operand"); - if (parser.parseOptionalAttrDict(result.attributes)) + SMLoc opLoc = parser.getCurrentLocation(); + if (parser.parseOperand(targetOperand) || + parser.parseOptionalAttrDict(result.attributes)) return failure(); Attribute sizesAttr = result.attributes.get(sizesAttrName); if (!sizesAttr) diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp --- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp +++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp @@ -154,8 +154,9 @@ static OptionalParseResult parserOptionalOperandAndTypeWithPrefix( OpAsmParser &parser, OperationState &result, StringRef prefixKeyword) { if (succeeded(parser.parseOptionalKeyword(prefixKeyword))) { - parser.parseEqual(); - return parseOperandAndType(parser, result); + if (parser.parseEqual() || parseOperandAndType(parser, result)) + return failure(); + return success(); } return llvm::None; } @@ -532,12 +533,14 @@ parser, result, LoopOp::getGangNumKeyword()); if (gangNum.hasValue() && failed(*gangNum)) return failure(); - parser.parseOptionalComma(); + // FIXME: Comma should require subsequent operands. + (void)parser.parseOptionalComma(); gangStatic = parserOptionalOperandAndTypeWithPrefix( parser, result, LoopOp::getGangStaticKeyword()); if (gangStatic.hasValue() && failed(*gangStatic)) return failure(); - parser.parseOptionalComma(); + // FIXME: Why allow optional last commas? + (void)parser.parseOptionalComma(); if (failed(parser.parseRParen())) return failure(); } diff --git a/mlir/lib/Dialect/Quant/IR/TypeParser.cpp b/mlir/lib/Dialect/Quant/IR/TypeParser.cpp --- a/mlir/lib/Dialect/Quant/IR/TypeParser.cpp +++ b/mlir/lib/Dialect/Quant/IR/TypeParser.cpp @@ -31,10 +31,8 @@ unsigned storageTypeWidth = 0; OptionalParseResult result = parser.parseOptionalType(type); if (result.hasValue()) { - if (!succeeded(*result)) { - parser.parseType(type); + if (!succeeded(*result)) return nullptr; - } isSigned = !type.isUnsigned(); storageTypeWidth = type.getWidth(); } else if (succeeded(parser.parseKeyword(&identifier))) { diff --git a/mlir/lib/Dialect/SCF/SCF.cpp b/mlir/lib/Dialect/SCF/SCF.cpp --- a/mlir/lib/Dialect/SCF/SCF.cpp +++ b/mlir/lib/Dialect/SCF/SCF.cpp @@ -2032,15 +2032,13 @@ static_cast(initVals.size())})); // Parse attributes. - if (parser.parseOptionalAttrDict(result.attributes)) + if (parser.parseOptionalAttrDict(result.attributes) || + parser.resolveOperands(initVals, result.types, parser.getNameLoc(), + result.operands)) return failure(); - if (!initVals.empty()) - parser.resolveOperands(initVals, result.types, parser.getNameLoc(), - result.operands); // Add a terminator if none was parsed. ForOp::ensureTerminator(*body, builder, result.location); - return success(); } diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp --- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp +++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp @@ -3112,15 +3112,15 @@ ParseResult spirv::ModuleOp::parse(OpAsmParser &parser, OperationState &state) { Region *body = state.addRegion(); + StringAttr nameAttr; + spirv::AddressingModel addrModel; + spirv::MemoryModel memoryModel; // If the name is present, parse it. - StringAttr nameAttr; - parser.parseOptionalSymbolName( + (void)parser.parseOptionalSymbolName( nameAttr, mlir::SymbolTable::getSymbolAttrName(), state.attributes); // Parse attributes - spirv::AddressingModel addrModel; - spirv::MemoryModel memoryModel; if (::parseEnumKeywordAttr(addrModel, parser, state) || ::parseEnumKeywordAttr(memoryModel, parser, state)) return failure(); @@ -3133,10 +3133,8 @@ return failure(); } - if (parser.parseOptionalAttrDictWithKeyword(state.attributes)) - return failure(); - - if (parser.parseRegion(*body, /*arguments=*/{}, /*argTypes=*/{})) + if (parser.parseOptionalAttrDictWithKeyword(state.attributes) || + parser.parseRegion(*body, /*arguments=*/{}, /*argTypes=*/{})) return failure(); // Make sure we have at least one block. diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp --- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp +++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp @@ -2864,7 +2864,8 @@ return failure(); ParseResult hasMask = parser.parseOptionalComma(); if (hasMask.succeeded()) { - parser.parseOperand(maskInfo); + if (parser.parseOperand(maskInfo)) + return failure(); } if (parser.parseOptionalAttrDict(result.attributes) || parser.getCurrentLocation(&typesLoc) || parser.parseColonTypeList(types)) diff --git a/mlir/lib/IR/FunctionImplementation.cpp b/mlir/lib/IR/FunctionImplementation.cpp --- a/mlir/lib/IR/FunctionImplementation.cpp +++ b/mlir/lib/IR/FunctionImplementation.cpp @@ -170,7 +170,7 @@ auto &builder = parser.getBuilder(); // Parse visibility. - impl::parseOptionalVisibilityKeyword(parser, result.attributes); + (void)impl::parseOptionalVisibilityKeyword(parser, result.attributes); // Parse the name as a symbol. StringAttr nameAttr; diff --git a/mlir/lib/Parser/AffineParser.cpp b/mlir/lib/Parser/AffineParser.cpp --- a/mlir/lib/Parser/AffineParser.cpp +++ b/mlir/lib/Parser/AffineParser.cpp @@ -50,9 +50,11 @@ : Parser(state), allowParsingSSAIds(allowParsingSSAIds), parseElement(parseElement) {} - AffineMap parseAffineMapRange(unsigned numDims, unsigned numSymbols); + ParseResult parseAffineMapRange(unsigned numDims, unsigned numSymbols, + AffineMap &result); ParseResult parseAffineMapOrIntegerSetInline(AffineMap &map, IntegerSet &set); - IntegerSet parseIntegerSetConstraints(unsigned numDims, unsigned numSymbols); + ParseResult parseIntegerSetConstraints(unsigned numDims, unsigned numSymbols, + IntegerSet &result); ParseResult parseAffineMapOfSSAIds(AffineMap &map, OpAsmParser::Delimiter delimiter); ParseResult parseAffineExprOfSSAIds(AffineExpr &expr); @@ -510,29 +512,15 @@ unsigned numDims = 0, numSymbols = 0; // List of dimensional and optional symbol identifiers. - if (parseDimAndOptionalSymbolIdList(numDims, numSymbols)) { - return failure(); - } - - // This is needed for parsing attributes as we wouldn't know whether we would - // be parsing an integer set attribute or an affine map attribute. - bool isArrow = getToken().is(Token::arrow); - bool isColon = getToken().is(Token::colon); - if (!isArrow && !isColon) - return emitWrongTokenError("expected '->' or ':'"); - - if (isArrow) { - parseToken(Token::arrow, "expected '->' or '['"); - map = parseAffineMapRange(numDims, numSymbols); - return map ? success() : failure(); - } - if (parseToken(Token::colon, "expected ':' or '['")) + if (parseDimAndOptionalSymbolIdList(numDims, numSymbols)) return failure(); - if ((set = parseIntegerSetConstraints(numDims, numSymbols))) - return success(); + if (consumeIf(Token::arrow)) + return parseAffineMapRange(numDims, numSymbols, map); - return failure(); + if (parseToken(Token::colon, "expected '->' or '['")) + return failure(); + return parseIntegerSetConstraints(numDims, numSymbols, set); } /// Parse an AffineMap where the dim and symbol identifiers are SSA ids. @@ -572,8 +560,9 @@ /// /// multi-dim-affine-expr ::= `(` `)` /// multi-dim-affine-expr ::= `(` affine-expr (`,` affine-expr)* `)` -AffineMap AffineParser::parseAffineMapRange(unsigned numDims, - unsigned numSymbols) { +ParseResult AffineParser::parseAffineMapRange(unsigned numDims, + unsigned numSymbols, + AffineMap &result) { SmallVector exprs; auto parseElt = [&]() -> ParseResult { auto elt = parseAffineExpr(); @@ -588,10 +577,11 @@ // | `(` affine-expr (`,` affine-expr)* `)` if (parseCommaSeparatedList(Delimiter::Paren, parseElt, " in affine map range")) - return AffineMap(); + return failure(); // Parsed a valid affine map. - return AffineMap::get(numDims, numSymbols, exprs, getContext()); + result = AffineMap::get(numDims, numSymbols, exprs, getContext()); + return success(); } /// Parse an affine constraint. @@ -639,8 +629,9 @@ /// affine-constraint-conjunction ::= affine-constraint (`,` /// affine-constraint)* /// -IntegerSet AffineParser::parseIntegerSetConstraints(unsigned numDims, - unsigned numSymbols) { +ParseResult AffineParser::parseIntegerSetConstraints(unsigned numDims, + unsigned numSymbols, + IntegerSet &result) { SmallVector constraints; SmallVector isEqs; auto parseElt = [&]() -> ParseResult { @@ -657,17 +648,19 @@ // Parse a list of affine constraints (comma-separated). if (parseCommaSeparatedList(Delimiter::Paren, parseElt, " in integer set constraint list")) - return IntegerSet(); + return failure(); // 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); + result = IntegerSet::get(numDims, numSymbols, zero, true); + return success(); } // Parsed a valid integer set. - return IntegerSet::get(numDims, numSymbols, constraints, isEqs); + result = IntegerSet::get(numDims, numSymbols, constraints, isEqs); + return success(); } //===----------------------------------------------------------------------===// diff --git a/mlir/test/Dialect/SCF/invalid.mlir b/mlir/test/Dialect/SCF/invalid.mlir --- a/mlir/test/Dialect/SCF/invalid.mlir +++ b/mlir/test/Dialect/SCF/invalid.mlir @@ -199,7 +199,7 @@ func.func @parallel_more_results_than_initial_values( %arg0 : index, %arg1: index, %arg2: index) { - // expected-error@+1 {{expects number of results: 1 to be the same as number of initial values: 0}} + // expected-error@+1 {{'scf.parallel' 0 operands present, but expected 1}} %res = scf.parallel (%i0) = (%arg0) to (%arg1) step (%arg2) -> f32 { scf.reduce(%arg0) : index { ^bb0(%lhs: index, %rhs: index): diff --git a/mlir/test/IR/invalid.mlir b/mlir/test/IR/invalid.mlir --- a/mlir/test/IR/invalid.mlir +++ b/mlir/test/IR/invalid.mlir @@ -932,7 +932,7 @@ func.func @invalid_affine_structure() { %c0 = arith.constant 0 : index - %idx = affine.apply affine_map<(d0, d1)> (%c0, %c0) // expected-error {{expected '->' or ':'}} + %idx = affine.apply affine_map<(d0, d1)> (%c0, %c0) // expected-error {{expected '->' or '['}} return } diff --git a/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp b/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp --- a/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp +++ b/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp @@ -212,12 +212,13 @@ return CustomDataLayoutSpec::get(parser.getContext(), {}); SmallVector entries; - parser.parseCommaSeparatedList([&]() { + ok = succeeded(parser.parseCommaSeparatedList([&]() { entries.emplace_back(); ok = succeeded(parser.parseAttribute(entries.back())); assert(ok); return success(); - }); + })); + assert(ok); ok = succeeded(parser.parseGreater()); assert(ok); return CustomDataLayoutSpec::get(parser.getContext(), entries);