[MLIR] Fix parseFunctionLikeOp() to fail parsing empty regions

[jurahul]

Summary

• For function like operations, empty regions indicate declarations, so we should not accept an empty region when parsing.
• See https://llvm.discourse.group/t/funcop-parsing-bug/2164

[jurahul]

Ping.

[rriddle]

Can't the users of parseRegion just check to see if it is empty after success and emit an error if they don't want that behavior?

[jurahul]

Not really. Note that both

func @foo()
func @foo() {}

post parsing look the same (the region attached to FuncOp is empty). So that's too late for the check.

mlir/lib/IR/FunctionImplementation.cpp

209–210

For reference, we cannot do the error check here after parser.parseOptionalRegion() because there is no way to distinguish between no region in the assembly (func @foo) vs an empty region (func @foo {}).

[rriddle]

mlir/lib/IR/FunctionImplementation.cpp

209–210

OptionalParseResult represents 3 different value states. Just change the other parseOptionalRegion to use OptionalParseResult (which it should be doing anyways). Then you could do the following:

OptionalParseResult result = parser.parseOptionalRegion(region);
if (result) {
  if (failed(*result))
    return failure();
  if (region.empty())
    ...
}
return success();

[jurahul]

River's feedback

[rriddle]

Nice!

flang/include/flang/Optimizer/Dialect/FIROps.td

if (parser.parseAttribute(nameAttr, mlir::SymbolTable::getSymbolAttrName(),
       return failure();
 
     // Convert the parsed name attr into a string attr.
     result.attributes.set(mlir::SymbolTable::getSymbolAttrName(),
       parser.getBuilder().getStringAttr(nameAttr.getRootReference()));
 
     // 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);
     return mlir::success();
   }];
 
   let printer = [{
     auto tableName = getAttrOfType<StringAttr>(

mlir/include/mlir/IR/OpImplementation.h

public:
   parseAffineMapOfSSAIds(SmallVectorImpl<OperandType> &operands, Attribute &map,
                          StringRef attrName, NamedAttrList &attrs,
                          Delimiter delimiter = Delimiter::Square) = 0;
 
   //===--------------------------------------------------------------------===//
   // 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 &region,
                                   ArrayRef<OperandType> arguments = {},
                                   ArrayRef<Type> argTypes = {},
                                   bool enableNameShadowing = false) = 0;
 
   /// Parses a region if present.
-  virtual ParseResult parseOptionalRegion(Region &region,
-                                          ArrayRef<OperandType> arguments = {},
-                                          ArrayRef<Type> argTypes = {},
-                                          bool enableNameShadowing = false) = 0;
+  virtual OptionalParseResult
+  parseOptionalRegion(Region &region, ArrayRef<OperandType> arguments = {},
 
   /// 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,
   /// `region` remains untouched.
   virtual OptionalParseResult parseOptionalRegion(
       std::unique_ptr<Region> &region, ArrayRef<OperandType> arguments = {},
       ArrayRef<Type> argTypes = {}, bool enableNameShadowing = false) = 0;
 

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

if (auto strAttr = value.dyn_cast_or_null<StringAttr>()) {
       MLIRContext *context = parser.getBuilder().getContext();
       auto arrayType = LLVM::LLVMType::getArrayTy(
           LLVM::LLVMType::getInt8Ty(context), strAttr.getValue().size());
       types.push_back(arrayType);
     } else {
       return parser.emitError(parser.getNameLoc(),
                               "type can only be omitted for string globals");
     }
-  } else if (parser.parseOptionalRegion(initRegion, /*arguments=*/{},
-                                        /*argTypes=*/{})) {
+  } else {
+    OptionalParseResult parseResult =
+        parser.parseOptionalRegion(initRegion, /*arguments=*/{},
+                                   /*argTypes=*/{});
+    if (parseResult.hasValue() && failed(*parseResult))
       return failure();
   }
 
   result.addAttribute("type", TypeAttr::get(types[0]));
   return success();
 }
 
 static LogicalResult verify(GlobalOp op) {
   if (!LLVMPointerType::isValidElementType(op.getType()))
static ParseResult parseLLVMFuncOp(OpAsmParser &parser,
   result.addAttribute(impl::getTypeAttrName(), TypeAttr::get(type));
 
   if (failed(parser.parseOptionalAttrDictWithKeyword(result.attributes)))
     return failure();
   impl::addArgAndResultAttrs(parser.getBuilder(), result, argAttrs,
                              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
 // helper functions. Drops "void" result since it cannot be parsed back. Skips
 // the external linkage since it is the default value.
 static void printLLVMFuncOp(OpAsmPrinter &p, LLVMFuncOp op) {
   p << op.getOperationName() << ' ';
   if (op.linkage() != LLVM::Linkage::External)

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

static ParseResult parseFuncOp(OpAsmParser &parser, OperationState &state) {
 
   // Add the attributes to the function arguments.
   assert(argAttrs.size() == argTypes.size());
   assert(resultAttrs.size() == resultTypes.size());
   impl::addArgAndResultAttrs(builder, state, argAttrs, resultAttrs);
 
   // 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) {
   // Print function name, signature, and control.
   printer << spirv::FuncOp::getOperationName() << " ";
   printer.printSymbolName(fnOp.sym_name());
   auto fnType = fnOp.getType();
   impl::printFunctionSignature(printer, fnOp, fnType.getInputs(),

mlir/lib/IR/FunctionImplementation.cpp

mlir::impl::parseFunctionLikeOp(OpAsmParser &parser, OperationState &result,
   if (parser.parseOptionalAttrDictWithKeyword(result.attributes))
     return failure();
 
   // Add the attributes to the function arguments.
   assert(argAttrs.size() == argTypes.size());
   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.
 static void printFunctionResultList(OpAsmPrinter &p, ArrayRef<Type> types,
                                     ArrayRef<ArrayRef<NamedAttribute>> attrs) {
   assert(!types.empty() && "Should not be called for empty result list.");
   auto &os = p.getStream();
   bool needsParens =

mlir/lib/Parser/Parser.cpp

assert((!enableNameShadowing ||
             opDefinition->hasProperty(OperationProperty::IsolatedFromAbove)) &&
            "name shadowing is only allowed on isolated regions");
     if (parser.parseRegion(region, regionArguments, enableNameShadowing))
       return failure();
     return success();
   }
 
   /// Parses a region if present.
-  ParseResult parseOptionalRegion(Region &region,
+  OptionalParseResult parseOptionalRegion(Region &region,
                                           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);
   }
 
   /// Parses a region if present. If the region is present, a new region is
   /// allocated and placed in `region`. If no region is present, `region`
   /// remains untouched.
   OptionalParseResult
   parseOptionalRegion(std::unique_ptr<Region> &region,

mlir/test/Dialect/SCF/canonicalize.mlir

 // CHECK:             scf.yield [[V1]] : index
 // CHECK:           } else
 // CHECK:             scf.yield [[C0]] : index
 // CHECK:           }
 // CHECK:           return [[V0]] : index
 
 // -----
 
-func private @side_effect() {}
+func private @side_effect()
 func @all_unused(%cond: i1) {
   %c0 = constant 0 : index
   %c1 = constant 1 : index
   %0, %1 = scf.if %cond -> (index, index) {
     call @side_effect() : () -> ()
     scf.yield %c0, %c1 : index, index
   } else {
     call @side_effect() : () -> ()

mlir/test/IR/invalid.mlir

"foo.region"() ({
       "foo.yield" () : () -> ()
     }, {
       // expected-error @+1 {{expected operation name in quotes}}
       %2 = constant 1 i32  // Syntax error causes region deletion.
     }) : () -> ()
   }
   return
 }
+
+// -----
+
+func @foo() {} // expected-error {{expected non-empty function body}}

mlir/test/IR/traits.mlir

 "test.symbol"() {sym_name = "foo_4", sym_visibility = "private"} : () -> ()
 "test.symbol"() {sym_name = "foo_5", sym_visibility = "public"} : () -> ()
 "test.symbol"() {sym_name = "foo_6"} : () -> ()
 
 // -----
 
 // 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() 
 
 // -----
 
 // Test that operation with the SymbolTable Trait fails with  too many blocks.
 // expected-error@+1 {{Operations with a 'SymbolTable' must have exactly one block}}
 "test.symbol_scope"() ({
   ^entry:
     "test.finish" () : () -> ()

mlir/tools/mlir-tblgen/OpFormatGen.cpp

const char *regionListEnsureTerminatorParserCode = R"(
   for (auto &region : {0}Regions)
     ensureTerminator(*region, parser.getBuilder(), result.location);
 )";
 
 /// The code snippet used to generate a parser call for an optional region.
 ///
 /// {0}: The name of the region.
 const char *optionalRegionParserCode = R"(
-  if (parser.parseOptionalRegion(*{0}Region))
+  {
+     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.
 ///
 /// {0}: The name of the region.
 const char *regionParserCode = R"(
   if (parser.parseRegion(*{0}Region))
     return ::mlir::failure();