[Mlir-commits] [mlir] fe7fdca - [MLIR] Fix parseFunctionLikeOp() to fail parsing empty regions
Rahul Joshi
llvmlistbot at llvm.org
Fri Dec 4 09:16:51 PST 2020
Author: Rahul Joshi
Date: 2020-12-04T09:09:59-08:00
New Revision: fe7fdcac87be4ad3de34901eb6fb5746437610e8
URL: https://github.com/llvm/llvm-project/commit/fe7fdcac87be4ad3de34901eb6fb5746437610e8
DIFF: https://github.com/llvm/llvm-project/commit/fe7fdcac87be4ad3de34901eb6fb5746437610e8.diff
LOG: [MLIR] Fix parseFunctionLikeOp() to fail parsing empty regions
- Change parseOptionalRegion to return an OptionalParseResult.
- Change parseFunctionLikeOp() to fail parsing if the function body was parsed but was
empty.
- See https://llvm.discourse.group/t/funcop-parsing-bug/2164
Differential Revision: https://reviews.llvm.org/D91886
Added:
Modified:
flang/include/flang/Optimizer/Dialect/FIROps.td
mlir/include/mlir/IR/OpImplementation.h
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
mlir/lib/IR/FunctionImplementation.cpp
mlir/lib/Parser/Parser.cpp
mlir/test/Dialect/SCF/canonicalize.mlir
mlir/test/IR/invalid.mlir
mlir/test/IR/traits.mlir
mlir/tools/mlir-tblgen/OpFormatGen.cpp
Removed:
################################################################################
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.td b/flang/include/flang/Optimizer/Dialect/FIROps.td
index c0448eca03dc..74471cf6f222 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.td
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.td
@@ -2856,7 +2856,8 @@ def fir_DispatchTableOp : fir_Op<"dispatch_table",
// Parse the optional table body.
mlir::Region *body = result.addRegion();
- if (parser.parseOptionalRegion(*body, llvm::None, llvm::None))
+ OptionalParseResult parseResult = parser.parseOptionalRegion(*body);
+ if (parseResult.hasValue() && failed(*parseResult))
return mlir::failure();
ensureTerminator(*body, parser.getBuilder(), result.location);
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index 5cf05e058166..a7e87dc0ab06 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -646,23 +646,23 @@ class OpAsmParser {
// Region Parsing
//===--------------------------------------------------------------------===//
- /// Parses a region. Any parsed blocks are appended to "region" and must be
+ /// Parses a region. Any parsed blocks are appended to 'region' and must be
/// moved to the op regions after the op is created. The first block of the
- /// region takes "arguments" of types "argTypes". If "enableNameShadowing" is
+ /// region takes 'arguments' of types 'argTypes'. If 'enableNameShadowing' is
/// set to true, the argument names are allowed to shadow the names of other
- /// existing SSA values defined above the region scope. "enableNameShadowing"
+ /// existing SSA values defined above the region scope. 'enableNameShadowing'
/// can only be set to true for regions attached to operations that are
- /// "IsolatedFromAbove".
+ /// 'IsolatedFromAbove.
virtual ParseResult parseRegion(Region ®ion,
ArrayRef<OperandType> arguments = {},
ArrayRef<Type> argTypes = {},
bool enableNameShadowing = false) = 0;
/// Parses a region if present.
- virtual ParseResult parseOptionalRegion(Region ®ion,
- ArrayRef<OperandType> arguments = {},
- ArrayRef<Type> argTypes = {},
- bool enableNameShadowing = false) = 0;
+ virtual OptionalParseResult
+ parseOptionalRegion(Region ®ion, ArrayRef<OperandType> arguments = {},
+ ArrayRef<Type> argTypes = {},
+ bool enableNameShadowing = false) = 0;
/// Parses a region if present. If the region is present, a new region is
/// allocated and placed in `region`. If no region is present or on failure,
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 76182c7868d7..f481b702822b 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -1187,9 +1187,12 @@ static ParseResult parseGlobalOp(OpAsmParser &parser, OperationState &result) {
return parser.emitError(parser.getNameLoc(),
"type can only be omitted for string globals");
}
- } else if (parser.parseOptionalRegion(initRegion, /*arguments=*/{},
- /*argTypes=*/{})) {
- return failure();
+ } else {
+ OptionalParseResult parseResult =
+ parser.parseOptionalRegion(initRegion, /*arguments=*/{},
+ /*argTypes=*/{});
+ if (parseResult.hasValue() && failed(*parseResult))
+ return failure();
}
result.addAttribute("type", TypeAttr::get(types[0]));
@@ -1398,8 +1401,9 @@ static ParseResult parseLLVMFuncOp(OpAsmParser &parser,
resultAttrs);
auto *body = result.addRegion();
- return parser.parseOptionalRegion(
+ OptionalParseResult parseResult = parser.parseOptionalRegion(
*body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes);
+ return failure(parseResult.hasValue() && failed(*parseResult));
}
// Print the LLVMFuncOp. Collects argument and result types and passes them to
diff --git a/mlir/lib/Dialect/SPIRV/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
index 6a13a5513fc7..776fbdf11c80 100644
--- a/mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
@@ -1754,8 +1754,9 @@ static ParseResult parseFuncOp(OpAsmParser &parser, OperationState &state) {
// Parse the optional function body.
auto *body = state.addRegion();
- return parser.parseOptionalRegion(
+ OptionalParseResult result = parser.parseOptionalRegion(
*body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes);
+ return failure(result.hasValue() && failed(*result));
}
static void print(spirv::FuncOp fnOp, OpAsmPrinter &printer) {
diff --git a/mlir/lib/IR/FunctionImplementation.cpp b/mlir/lib/IR/FunctionImplementation.cpp
index e93c91fdd342..90ea91d49fb6 100644
--- a/mlir/lib/IR/FunctionImplementation.cpp
+++ b/mlir/lib/IR/FunctionImplementation.cpp
@@ -204,10 +204,21 @@ mlir::impl::parseFunctionLikeOp(OpAsmParser &parser, OperationState &result,
assert(resultAttrs.size() == resultTypes.size());
addArgAndResultAttrs(builder, result, argAttrs, resultAttrs);
- // Parse the optional function body.
+ // Parse the optional function body. The printer will not print the body if
+ // its empty, so disallow parsing of empty body in the parser.
auto *body = result.addRegion();
- return parser.parseOptionalRegion(
- *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes);
+ llvm::SMLoc loc = parser.getCurrentLocation();
+ OptionalParseResult parseResult = parser.parseOptionalRegion(
+ *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes,
+ /*enableNameShadowing=*/false);
+ if (parseResult.hasValue()) {
+ if (failed(*parseResult))
+ return failure();
+ // Function body was parsed, make sure its not empty.
+ if (body->empty())
+ return parser.emitError(loc, "expected non-empty function body");
+ }
+ return success();
}
// Print a function result list.
diff --git a/mlir/lib/Parser/Parser.cpp b/mlir/lib/Parser/Parser.cpp
index a5beb559f75b..47fef1ca393c 100644
--- a/mlir/lib/Parser/Parser.cpp
+++ b/mlir/lib/Parser/Parser.cpp
@@ -1433,12 +1433,12 @@ class CustomOpAsmParser : public OpAsmParser {
}
/// Parses a region if present.
- ParseResult parseOptionalRegion(Region ®ion,
- ArrayRef<OperandType> arguments,
- ArrayRef<Type> argTypes,
- bool enableNameShadowing) override {
+ OptionalParseResult parseOptionalRegion(Region ®ion,
+ ArrayRef<OperandType> arguments,
+ ArrayRef<Type> argTypes,
+ bool enableNameShadowing) override {
if (parser.getToken().isNot(Token::l_brace))
- return success();
+ return llvm::None;
return parseRegion(region, arguments, argTypes, enableNameShadowing);
}
diff --git a/mlir/test/Dialect/SCF/canonicalize.mlir b/mlir/test/Dialect/SCF/canonicalize.mlir
index d57563461241..05b75a55da8f 100644
--- a/mlir/test/Dialect/SCF/canonicalize.mlir
+++ b/mlir/test/Dialect/SCF/canonicalize.mlir
@@ -86,7 +86,7 @@ func @nested_unused(%cond1: i1, %cond2: i1) -> (index) {
// -----
-func private @side_effect() {}
+func private @side_effect()
func @all_unused(%cond: i1) {
%c0 = constant 0 : index
%c1 = constant 1 : index
diff --git a/mlir/test/IR/invalid.mlir b/mlir/test/IR/invalid.mlir
index 4dfbfa7133c1..4930341a8b64 100644
--- a/mlir/test/IR/invalid.mlir
+++ b/mlir/test/IR/invalid.mlir
@@ -1572,3 +1572,7 @@ func @invalid_region_dominance_with_dominance_free_regions() {
}
return
}
+
+// -----
+
+func @foo() {} // expected-error {{expected non-empty function body}}
diff --git a/mlir/test/IR/traits.mlir b/mlir/test/IR/traits.mlir
index 88762812e311..c023db338a73 100644
--- a/mlir/test/IR/traits.mlir
+++ b/mlir/test/IR/traits.mlir
@@ -344,12 +344,10 @@ func @failedSingleBlockImplicitTerminator_missing_terminator() {
// Test that operation with the SymbolTable Trait define a new symbol scope.
"test.symbol_scope"() ({
- func private @foo() {
- }
+ func private @foo()
"test.finish" () : () -> ()
}) : () -> ()
-func private @foo() {
-}
+func private @foo()
// -----
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index 937ee7747af3..e5775c09ab25 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -652,8 +652,11 @@ const char *regionListEnsureTerminatorParserCode = R"(
///
/// {0}: The name of the region.
const char *optionalRegionParserCode = R"(
- if (parser.parseOptionalRegion(*{0}Region))
- return ::mlir::failure();
+ {
+ auto parseResult = parser.parseOptionalRegion(*{0}Region);
+ if (parseResult.hasValue() && failed(*parseResult))
+ return ::mlir::failure();
+ }
)";
/// The code snippet used to generate a parser call for a region.
More information about the Mlir-commits
mailing list