[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

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

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!

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. If 'allowEmptyRegion' is false, parsing will fail if
+  /// the region is empty.
   virtual ParseResult parseRegion(Region &region,
                                   ArrayRef<OperandType> arguments = {},
                                   ArrayRef<Type> argTypes = {},
-                                  bool enableNameShadowing = false) = 0;
+                                  bool enableNameShadowing = false,
+                                  bool allowEmptyRegion = true) = 0;
 
   /// Parses a region if present.
   virtual ParseResult parseOptionalRegion(Region &region,
                                           ArrayRef<OperandType> arguments = {},
                                           ArrayRef<Type> argTypes = {},
-                                          bool enableNameShadowing = false) = 0;
+                                          bool enableNameShadowing = false,
+                                          bool allowEmptyRegion = true) = 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,
   /// `region` remains untouched.
   virtual OptionalParseResult parseOptionalRegion(
       std::unique_ptr<Region> &region, ArrayRef<OperandType> arguments = {},
-      ArrayRef<Type> argTypes = {}, bool enableNameShadowing = false) = 0;
+      ArrayRef<Type> argTypes = {}, bool enableNameShadowing = false,
+      bool allowEmptyRegion = true) = 0;
 
   /// Parse a region argument, this argument is resolved when calling
   /// 'parseRegion'.
   virtual ParseResult parseRegionArgument(OperandType &argument) = 0;
 
   /// Parse zero or more region arguments with a specified surrounding
   /// delimiter, and an optional required argument count. Region arguments
   /// define new values; so this also checks if values with the same names have

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);
+      *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes,
+      /*enableNameShadowing=*/false,
+      /*allowEmptyRegion=*/false);
 }
 
 // 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

public:
   Operation *parseCustomOperation(ArrayRef<ResultRecord> resultIDs);
 
   //===--------------------------------------------------------------------===//
   // Region Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a region into 'region' with the provided entry block arguments.
   /// 'isIsolatedNameScope' indicates if the naming scope of this region is
-  /// isolated from those above.
+  /// isolated from those above. If 'allowEmptyRegion' is false, parsing will
+  /// fail if the region is empty.
   ParseResult parseRegion(Region &region,
                           ArrayRef<std::pair<SSAUseInfo, Type>> entryArguments,
-                          bool isIsolatedNameScope = false);
+                          bool isIsolatedNameScope = false,
+                          bool allowEmptyRegion = true);
 
   /// Parse a region body into 'region'.
   ParseResult parseRegionBody(Region &region);
 
   //===--------------------------------------------------------------------===//
   // Block Parsing
   //===--------------------------------------------------------------------===//
 
public:
 
   //===--------------------------------------------------------------------===//
   // Region Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a region that takes `arguments` of `argTypes` types.  This
   /// effectively defines the SSA values of `arguments` and assigns their type.
   ParseResult parseRegion(Region &region, ArrayRef<OperandType> arguments,
-                          ArrayRef<Type> argTypes,
-                          bool enableNameShadowing) override {
+                          ArrayRef<Type> argTypes, bool enableNameShadowing,
+                          bool allowEmptyRegion) override {
     assert(arguments.size() == argTypes.size() &&
            "mismatching number of arguments and types");
 
     SmallVector<std::pair<OperationParser::SSAUseInfo, Type>, 2>
         regionArguments;
     for (auto pair : llvm::zip(arguments, argTypes)) {
       const OperandType &operand = std::get<0>(pair);
       Type type = std::get<1>(pair);
       OperationParser::SSAUseInfo operandInfo = {operand.name, operand.number,
                                                  operand.location};
       regionArguments.emplace_back(operandInfo, type);
     }
 
     // Try to parse the region.
     assert((!enableNameShadowing ||
             opDefinition->hasProperty(OperationProperty::IsolatedFromAbove)) &&
            "name shadowing is only allowed on isolated regions");
-    if (parser.parseRegion(region, regionArguments, enableNameShadowing))
+    if (parser.parseRegion(region, regionArguments, enableNameShadowing,
+                           allowEmptyRegion))
       return failure();
     return success();
   }
 
   /// Parses a region if present.
   ParseResult parseOptionalRegion(Region &region,
                                   ArrayRef<OperandType> arguments,
                                   ArrayRef<Type> argTypes,
-                                  bool enableNameShadowing) override {
+                                  bool enableNameShadowing,
+                                  bool allowEmptyRegion) override {
     if (parser.getToken().isNot(Token::l_brace))
       return success();
-    return parseRegion(region, arguments, argTypes, enableNameShadowing);
+    return parseRegion(region, arguments, argTypes, enableNameShadowing,
+                       allowEmptyRegion);
   }
 
   /// 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,
-                      ArrayRef<OperandType> arguments, ArrayRef<Type> argTypes,
-                      bool enableNameShadowing = false) override {
+  OptionalParseResult parseOptionalRegion(std::unique_ptr<Region> &region,
+                                          ArrayRef<OperandType> arguments,
+                                          ArrayRef<Type> argTypes,
+                                          bool enableNameShadowing,
+                                          bool allowEmptyRegion) override {
     if (parser.getToken().isNot(Token::l_brace))
       return llvm::None;
     std::unique_ptr<Region> newRegion = std::make_unique<Region>();
-    if (parseRegion(*newRegion, arguments, argTypes, enableNameShadowing))
+    if (parseRegion(*newRegion, arguments, argTypes, enableNameShadowing,
+                    allowEmptyRegion))
       return failure();
 
     region = std::move(newRegion);
     return success();
   }
 
   /// Parse a region argument. The type of the argument will be resolved later
   /// by a call to `parseRegion`.

 
 /// Region.
 ///
 ///   region ::= '{' region-body
 ///
 ParseResult OperationParser::parseRegion(
     Region &region,
     ArrayRef<std::pair<OperationParser::SSAUseInfo, Type>> entryArguments,
-    bool isIsolatedNameScope) {
+    bool isIsolatedNameScope, bool allowEmptyRegion) {
+  llvm::SMLoc regionLoc = getToken().getLoc();
+
   // Parse the '{'.
   if (parseToken(Token::l_brace, "expected '{' to begin a region"))
     return failure();
 
   // Check for an empty region.
-  if (entryArguments.empty() && consumeIf(Token::r_brace))
+  if (entryArguments.empty() && consumeIf(Token::r_brace)) {
+    if (allowEmptyRegion)
       return success();
+    return emitError(regionLoc, "expected non-empty region");
+  }
   auto currentPt = opBuilder.saveInsertionPoint();
 
   // Push a new named value scope.
   pushSSANameScope(isIsolatedNameScope);
 
   // Parse the first block directly to allow for it to be unnamed.
   auto owning_block = std::make_unique<Block>();
   Block *block = owning_block.get();

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() {
   %c0 = constant 0 : index
   %c1 = constant 1 : index
   %true = constant true
   %0, %1 = scf.if %true -> (index, index) {
     call @side_effect() : () -> ()
     scf.yield %c0, %c1 : index, index
   } else {

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 region}}

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" () : () -> ()