Change syntax of regions in the generic form of operations

The generic form of operations currently supports optional regions to be
    located after the operation type.  As we are going to add a type to each
    region in a leading position in the region syntax, similarly to functions, it
    becomes ambiguous to have regions immediately after the operation type.  Put
    regions between operands the optional list of successors in the generic
    operation syntax and wrap them in parentheses.  The effect on the exisitng IR
    syntax is minimal since only three operations (`affine.for`, `affine.if` and
    `gpu.kernel`) currently use regions.

--

PiperOrigin-RevId: 246787087

mlir/bindings/python/test/test_py2and3.py

@@ -69,12 +69,14 @@ class EdscTest(unittest.TestCase):
           x = i + j
     code = str(fun)
     # TODO(zinenko,ntv): use FileCheck for these tests
+    self.assertIn('  "affine.for"() ( {\n', code)
     self.assertIn(
-        '  "affine.for"() {lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (42)} : () -> () {\n',
+        "{lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (42)}",
         code)
     self.assertIn("  ^bb1(%i0: index):", code)
+    self.assertIn('    "affine.for"(%c42, %2) ( {\n', code)
     self.assertIn(
-        '    "affine.for"(%c42, %2) {lower_bound: (d0) -> (d0), step: 2 : index, upper_bound: (d0) -> (d0)} : (index, index) -> () {\n',
+        "{lower_bound: (d0) -> (d0), step: 2 : index, upper_bound: (d0) -> (d0)} : (index, index) -> ()",
         code)
     self.assertIn("    ^bb2(%i1: index):", code)
     self.assertIn(
@@ -89,21 +91,13 @@ class EdscTest(unittest.TestCase):
         i + j + k + l
 
     code = str(fun)
-    self.assertIn(
+    self.assertIn(' "affine.for"() ( {\n', code)
-        ' "affine.for"() {lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (5)} : () -> () {\n',
-        code)
     self.assertIn("  ^bb1(%i0: index):", code)
-    self.assertIn(
+    self.assertIn('    "affine.for"() ( {\n', code)
-        '    "affine.for"() {lower_bound: () -> (1), step: 3 : index, upper_bound: () -> (15)} : () -> () {\n',
-        code)
     self.assertIn("    ^bb2(%i1: index):", code)
-    self.assertIn(
+    self.assertIn('      "affine.for"() ( {\n', code)
-        '      "affine.for"() {lower_bound: () -> (2), step: 5 : index, upper_bound: () -> (25)} : () -> () {\n',
-        code)
     self.assertIn("      ^bb3(%i2: index):", code)
-    self.assertIn(
+    self.assertIn('        "affine.for"() ( {\n', code)
-        '        "affine.for"() {lower_bound: () -> (3), step: 7 : index, upper_bound: () -> (35)} : () -> () {\n',
-        code)
     self.assertIn("        ^bb4(%i3: index):", code)
     self.assertIn(
         '          %2 = "affine.apply"(%i0, %i1, %i2, %i3) {map: (d0, d1, d2, d3) -> (d0 + d1 + d2 + d3)} : (index, index, index, index) -> index',
@@ -367,11 +361,13 @@ class EdscTest(unittest.TestCase):
       E.ret([fun.arg(0)])
 
     code = str(fun)
+    self.assertIn('"affine.for"()', code)
     self.assertIn(
-        '"affine.for"() {lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (10)}',
+        "{lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (10)}",
         code)
+    self.assertIn('"affine.for"()', code)
     self.assertIn(
-        '"affine.for"() {lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (42)}',
+        "{lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (42)}",
         code)
     self.assertIn("%0 = load %arg0[%i0, %i1] : memref<10x42xf32>", code)
     self.assertIn("%1 = addf %0, %cst : f32", code)
@@ -392,8 +388,9 @@ class EdscTest(unittest.TestCase):
       E.ret([])
 
     code = str(fun)
+    self.assertIn('"affine.for"()', code)
     self.assertIn(
-        '"affine.for"() {lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (32)} : () -> ()',
+        "{lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (32)} : () -> ()",
         code)
     self.assertIn("%0 = load %arg0[%i0, %i2] : memref<32x32xf32>", code)
     self.assertIn("%1 = load %arg1[%i2, %i1] : memref<32x32xf32>", code)

mlir/g3doc/LangRef.md

@@ -1413,9 +1413,11 @@ Syntax:
 
 ``` {.ebnf}
 operation ::= op-result? string-literal `(` ssa-use-list? `)`
-              (`[` successor-list `]`)? attribute-dict? `:` function-type
+              (`[` successor-list `]`)? (`(` region-list `)`)?
+              attribute-dict? `:` function-type
 op-result ::= ssa-id ((`:` integer-literal) | (`,` ssa-id)*) `=`
 successor-list ::= successor (`,` successor)*
+region-list    ::= region (`,` region)*
 ```
 
 MLIR represents computations within functions with a uniform concept called
@@ -1428,7 +1430,8 @@ operations), and have application-specific semantics. For example, MLIR supports
 The internal representation of an operation is simple: an operation is
 identified by a unique string (e.g. `dim`, `tf.Conv2d`, `x86.repmovsb`,
 `ppc.eieio`, etc), can return zero or more results, take zero or more SSA
-operands, and may have zero or more attributes. When parsed or printed in the
+operands, may have zero or more attributes, may have zero or more successors,
+and zero or more enclosed [regions](#regions). When parsed or printed in the
 _generic assembly form_, these are all printed literally, and a function type is
 used to indicate the types of the results and operands.
 

mlir/lib/IR/AsmPrinter.cpp

@@ -1582,6 +1582,16 @@ void FunctionPrinter::printGenericOp(Operation *op) {
     os << ']';
   }
 
+  // Print regions.
+  if (op->getNumRegions() != 0) {
+    os << " (";
+    interleaveComma(op->getRegions(), [&](Region &region) {
+      printRegion(region, /*printEntryBlockArgs=*/true,
+                  /*printBlockTerminators=*/true);
+    });
+    os << ')';
+  }
+
   auto attrs = op->getAttrs();
   printOptionalAttrDict(attrs);
 
@@ -1600,11 +1610,6 @@ void FunctionPrinter::printGenericOp(Operation *op) {
                     [&](Value *result) { printType(result->getType()); });
     os << ')';
   }
-
-  // Print any trailing regions.
-  for (auto &region : op->getRegions())
-    printRegion(region, /*printEntryBlockArgs=*/true,
-                /*printBlockTerminators=*/true);
 }
 
 void FunctionPrinter::printSuccessorAndUseList(Operation *term,

mlir/lib/Parser/Parser.cpp

@@ -3083,6 +3083,20 @@ Operation *FunctionParser::parseGenericOperation() {
       return nullptr;
   }
 
+  // Parse the region list.
+  CleanupOpStateRegions guard{result};
+  if (consumeIf(Token::l_paren)) {
+    do {
+      // Create temporary regions with function as parent.
+      result.regions.emplace_back(new Region(function));
+      if (parseOperationRegion(*result.regions.back(),
+                               /*entryArguments*/ {}))
+        return nullptr;
+    } while (consumeIf(Token::comma));
+    if (parseToken(Token::r_paren, "expected ')' to end region list"))
+      return nullptr;
+  }
+
   if (getToken().is(Token::l_brace)) {
     if (parseAttributeDict(result.attributes))
       return nullptr;
@@ -3125,16 +3139,6 @@ Operation *FunctionParser::parseGenericOperation() {
     result.addSuccessor(successor, operands);
   }
 
-  // Parse the optional regions for this operation.
-  CleanupOpStateRegions guard{result};
-  while (getToken().is(Token::l_brace)) {
-    // Create temporary regions with function as parent.
-    result.regions.emplace_back(new Region(function));
-    if (parseOperationRegion(*result.regions.back(),
-                             /*entryArguments=*/llvm::None))
-      return nullptr;
-  }
-
   return builder.createOperation(result);
 }
 

mlir/test/AffineOps/ops.mlir

@@ -11,7 +11,7 @@ func @empty() {
   // GENERIC:      "affine.for"()
   // GENERIC-NEXT: ^bb1(%i0: index):
   // GENERIC-NEXT:   "affine.terminator"() : () -> ()
-  // GENERIC-NEXT: }
+  // GENERIC-NEXT: })
   affine.for %i = 0 to 10 {
   } {some_attr: true}
 
@@ -20,8 +20,8 @@ func @empty() {
   //
   // GENERIC:      "affine.if"()
   // GENERIC-NEXT:   "affine.terminator"() : () -> ()
-  // GENERIC-NEXT: } {
+  // GENERIC-NEXT: },  {
-  // GENERIC-NEXT: }
+  // GENERIC-NEXT: })
   affine.if () : () () {
   } {some_attr: true}
 
@@ -30,10 +30,10 @@ func @empty() {
   //
   // GENERIC:      "affine.if"()
   // GENERIC-NEXT:   "affine.terminator"() : () -> ()
-  // GENERIC-NEXT: } {
+  // GENERIC-NEXT: },  {
   // GENERIC-NEXT:   "foo"() : () -> ()
   // GENERIC-NEXT:   "affine.terminator"() : () -> ()
-  // GENERIC-NEXT: }
+  // GENERIC-NEXT: })
   affine.if () : () () {
   } else {
     "foo"() : () -> ()
@@ -49,10 +49,10 @@ func @affine_terminator() {
   // CHECK: affine.for %i
   // CHECK-NEXT: }
   //
-  // GENERIC:      "affine.for"() {lower_bound: #map0, step: 1 : index, upper_bound: #map1} : () -> () {
+  // GENERIC:      "affine.for"() ( {
   // GENERIC-NEXT: ^bb1(%i0: index):	// no predecessors
   // GENERIC-NEXT:   "affine.terminator"() : () -> ()
-  // GENERIC-NEXT: }
+  // GENERIC-NEXT: }) {lower_bound: #map0, step: 1 : index, upper_bound: #map1} : () -> ()
   affine.for %i = 0 to 10 {
     "affine.terminator"() : () -> ()
   }

mlir/test/GPU/invalid.mlir

@@ -2,10 +2,9 @@
 
 func @not_enough_sizes(%sz : index) {
   // expected-error@+1 {{expected 6 or more operands}}
-  "gpu.launch"(%sz, %sz, %sz, %sz, %sz)
+  "gpu.launch"(%sz, %sz, %sz, %sz, %sz) ({
-      : (index, index, index, index, index) -> () {
     return
-  }
+  }) : (index, index, index, index, index) -> ()
   return
 }
 
@@ -13,12 +12,11 @@ func @not_enough_sizes(%sz : index) {
 
 func @no_region_attrs(%sz : index) {
   // expected-error@+1 {{unexpected number of region arguments}}
- "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz)
+ "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({
-      : (index, index, index, index, index, index) -> () {
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index):
     return
-  }
+  }) : (index, index, index, index, index, index) -> ()
   return
 }
 
@@ -26,8 +24,7 @@ func @no_region_attrs(%sz : index) {
 
 func @isolation_arg(%sz : index) {
  // expected-note@+1 {{required by region isolation constraints}}
- "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz)
+ "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({
-      : (index, index, index, index, index, index) -> () {
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
        %szbx: index, %szby: index, %szbz: index,
@@ -35,7 +32,7 @@ func @isolation_arg(%sz : index) {
     // expected-error@+1 {{using value defined outside the region}}
     "use"(%sz) : (index) -> ()
     return
-  }
+  }) : (index, index, index, index, index, index) -> ()
   return
 }
 
@@ -44,8 +41,7 @@ func @isolation_arg(%sz : index) {
 func @isolation_op(%sz : index) {
  %val = "produce"() : () -> (index)
  // expected-note@+1 {{required by region isolation constraints}}
- "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz)
+ "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({
-      : (index, index, index, index, index, index) -> () {
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
        %szbx: index, %szby: index, %szbz: index,
@@ -53,7 +49,7 @@ func @isolation_op(%sz : index) {
     // expected-error@+1 {{using value defined outside the region}}
     "use"(%val) : (index) -> ()
     return
-  }
+  }) : (index, index, index, index, index, index) -> ()
   return
 }
 
@@ -61,18 +57,17 @@ func @isolation_op(%sz : index) {
 
 func @nested_isolation(%sz : index) {
   // expected-note@+1 {{required by region isolation constraints}}
-  "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz)
+  "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({
-      : (index, index, index, index, index, index) -> () {
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
        %szbx: index, %szby: index, %szbz: index,
        %sztx: index, %szty: index, %sztz: index):
-    "region"() : () -> () {
+    "region"() ({
-      "region"() : () -> () {
+      "region"() ({
         // expected-error@+1 {{using value defined outside the region}}
         "use"(%sz) : (index) -> ()
-      }
+      }) : () -> ()
-    }
+    }) : () -> ()
-  }
+  }) : (index, index, index, index, index, index) -> ()
   return
 }

mlir/test/GPU/ops.mlir

@@ -2,39 +2,41 @@
 
 // CHECK-LABEL:func @no_args(%arg0: index)
 func @no_args(%sz : index) {
-// CHECK:  "gpu.launch"(%arg0, %arg0, %arg0, %arg0, %arg0, %arg0) : (index, index, index, index, index, index) -> () {
+// CHECK:  "gpu.launch"(%arg0, %arg0, %arg0, %arg0, %arg0, %arg0)
-  "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz)
+// CHECK-SAME: {
-      : (index, index, index, index, index, index) -> () {
+  "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
        %szbx: index, %szby: index, %szbz: index,
        %sztx: index, %szty: index, %sztz: index):
     return
-  }
+// CHECK: (index, index, index, index, index, index) -> ()
+  }) : (index, index, index, index, index, index) -> ()
   return
 }
 
 // CHECK-LABEL:func @args(%arg0: index, %arg1: index, %arg2: f32, %arg3: memref<?xf32, 1>) {
 func @args(%blk : index, %thrd : index, %float : f32, %data : memref<?xf32,1>) {
-// CHECK:  "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3) : (index, index, index, index, index, index, f32, memref<?xf32, 1>) -> () {
+// CHECK:  "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3)
-  "gpu.launch"(%blk, %blk, %blk, %thrd, %thrd, %thrd, %float, %data)
+// CHECK-SAME: {
-      : (index, index, index, index, index, index, f32, memref<?xf32,1>) -> () {
+  "gpu.launch"(%blk, %blk, %blk, %thrd, %thrd, %thrd, %float, %data) ({
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
        %szbx: index, %szby: index, %szbz: index,
        %sztx: index, %szty: index, %sztz: index,
        %data0: f32, %data1: memref<?xf32,1>):
     return
-  }
+// CHECK: (index, index, index, index, index, index, f32, memref<?xf32, 1>) -> ()
+  }) : (index, index, index, index, index, index, f32, memref<?xf32,1>) -> ()
   return
 }
 
 // It is possible to use values passed into the region as arguments.
 // CHECK-LABEL: func @passing_values
 func @passing_values(%blk : index, %thrd : index, %float : f32, %data : memref<?xf32,1>) {
-// CHECK:  "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3) : (index, index, index, index, index, index, f32, memref<?xf32, 1>) -> () {
+// CHECK:  "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3)
-  "gpu.launch"(%blk, %blk, %blk, %thrd, %thrd, %thrd, %float, %data)
+// CHECK-SAME: {
-      : (index, index, index, index, index, index, f32, memref<?xf32,1>) -> () {
+  "gpu.launch"(%blk, %blk, %blk, %thrd, %thrd, %thrd, %float, %data) ({
 // CHECK: ^bb1(%i0: index, %i1: index, %i2: index, %i3: index, %i4: index, %i5: index, %i6: index, %i7: index, %i8: index, %i9: index, %i10: index, %i11: index, %i12: f32, %i13: memref<?xf32, 1>)
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
@@ -44,7 +46,7 @@ func @passing_values(%blk : index, %thrd : index, %float : f32, %data : memref<?
 // CHECK: "use"(%i12)
     "use"(%data0): (f32) -> ()
     return
-  }
+  }) : (index, index, index, index, index, index, f32, memref<?xf32,1>) -> ()
   return
 }
 
@@ -52,20 +54,19 @@ func @passing_values(%blk : index, %thrd : index, %float : f32, %data : memref<?
 // cross kernel launch region boundaries.
 // CHECK-LABEL: func @nested_isolation
 func @nested_isolation(%sz : index) {
-  "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz)
+  "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) ({
-      : (index, index, index, index, index, index) -> () {
   ^bb1(%bx: index, %by: index, %bz: index,
        %tx: index, %ty: index, %tz: index,
        %szbx: index, %szby: index, %szbz: index,
        %sztx: index, %szty: index, %sztz: index):
-    "region"() : () -> () {
+    "region"() ({
 // CHECK: %0 = "produce"()
       %val = "produce"() : () -> (index)
-      "region"() : () -> () {
+      "region"() ({
 // CHECK: "use"(%0)
         "use"(%val) : (index) -> ()
-      }
+      }) : () -> ()
-    }
+    }) : () -> ()
-  }
+  }) : (index, index, index, index, index, index) -> ()
   return
 }

mlir/test/IR/invalid.mlir

@@ -882,7 +882,7 @@ func @negative_in_tensor_size() -> tensor<1x-1xi32>
 // -----
 
 func @invalid_nested_dominance() {
-  "foo.region"() : () -> () {
+  "foo.region"() ({
     // expected-error @+1 {{operand #0 does not dominate this use}}
     "foo.use" (%1) : (i32) -> ()
     br ^bb2
@@ -891,7 +891,7 @@ func @invalid_nested_dominance() {
     // expected-note @+1 {{operand defined here}}
     %1 = constant 0 : i32
     "foo.yield" () : () -> ()
-  }
+  }) : () -> ()
   return
 }
 
@@ -925,13 +925,13 @@ func @invalid_tuple_missing_greater(tuple<i32)
 // Should not crash because of deletion order here.
 func @invalid_region_dominance() {
   "foo.use" (%1) : (i32) -> ()
-  "foo.region"() : () -> () {
+  "foo.region"() ({
     %1 = constant 0 : i32  // This value is used outside of the region.
     "foo.yield" () : () -> ()
-  } {
+  }, {
     // expected-error @+1 {{expected operation name in quotes}}
     %2 = constant 1 i32  // Syntax error causes region deletion.
-  }
+  }) : () -> ()
   return
 }
 
@@ -942,13 +942,13 @@ func @invalid_region_block() {
   "foo.branch"()[^bb2] : () -> ()  // Attempt to jump into the region.
 
 ^bb1:
-  "foo.region"() : () -> () {
+  "foo.region"() ({
     ^bb2:
       "foo.yield"() : () -> ()
-  } {
+  }, {
     // expected-error @+1 {{expected operation name in quotes}}
     %2 = constant 1 i32  // Syntax error causes region deletion.
-  }
+  }) : () -> ()
 }
 
 // -----
@@ -956,20 +956,27 @@ func @invalid_region_block() {
 // Should not crash because of deletion order here.
 func @invalid_region_dominance() {
   "foo.use" (%1) : (i32) -> ()
-  "foo.region"() : () -> () {
+  "foo.region"() ({
-    "foo.region"() : () -> () {
+    "foo.region"() ({
       %1 = constant 0 : i32  // This value is used outside of the region.
       "foo.yield" () : () -> ()
-    }
+    }) : () -> ()
-  } {
+  }, {
     // expected-error @+1 {{expected operation name in quotes}}
     %2 = constant 1 i32  // Syntax error causes region deletion.
-  }
+  }) : () -> ()
   return
 }
 
 // -----
 
+func @unfinished_region_list() {
+  // expected-error@+1 {{expected ')' to end region list}}
+  "region"() ({},{},{} : () -> ()
+}
+
+// -----
+
 func @multi_result_missing_count() {
   // expected-error@+1 {{expected integer number of results}}
   %0: = "foo" () : () -> (i32, i32)

mlir/test/IR/parser.mlir

@@ -800,16 +800,27 @@ func @verbose_if(%N: index) {
   %c = constant 200 : index
 
   // CHECK: affine.if #set{{.*}}(%c200)[%arg0, %c200] {
-  "affine.if"(%c, %N, %c) { condition: #set0 } : (index, index, index) -> () {
+  "affine.if"(%c, %N, %c) ({
     // CHECK-NEXT: "add"
     %y = "add"(%c, %N) : (index, index) -> index
     "affine.terminator"() : () -> ()
     // CHECK-NEXT: } else {
-  } { // The else region.
+  }, { // The else region.
     // CHECK-NEXT: "add"
     %z = "add"(%c, %c) : (index, index) -> index
     "affine.terminator"() : () -> ()
-  }
+  })
+  { condition: #set0 } : (index, index, index) -> ()
+  return
+}
+
+// CHECK-LABEL: func @terminator_with_regions
+func @terminator_with_regions() {
+  // Combine successors and regions in the same operation.
+  // CHECK: "region"()[^bb1] ( {
+  // CHECK: }) : () -> ()
+  "region"()[^bb2] ({}) : () -> ()
+^bb2:
   return
 }
 

mlir/test/Transforms/cse.mlir

@@ -193,7 +193,7 @@ func @up_propagate() -> i32 {
 // CHECK-LABEL: func @up_propagate_region
 func @up_propagate_region() -> i32 {
   // CHECK-NEXT: %0 = "foo.region"
-  %0 = "foo.region"() : () -> (i32) {
+  %0 = "foo.region"() ({
     // CHECK-NEXT:  %c0_i32 = constant 0 : i32
     // CHECK-NEXT: %true = constant 1 : i1
     // CHECK-NEXT: cond_br
@@ -217,6 +217,6 @@ func @up_propagate_region() -> i32 {
     %c1_i32_0 = constant 1 : i32
     %2 = addi %arg, %c1_i32_0 : i32
     "foo.yield" (%2) : (i32) -> ()
-  }
+  }) : () -> (i32)
   return %0 : i32
 }