From d3380a504fed2f521becb2ccbab81e70e53ceb65 Mon Sep 17 00:00:00 2001 From: Alex Zinenko Date: Mon, 6 May 2019 01:40:13 -0700 Subject: [PATCH] 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 | 31 +++++++++++------------- mlir/g3doc/LangRef.md | 7 ++++-- mlir/lib/IR/AsmPrinter.cpp | 15 ++++++++---- mlir/lib/Parser/Parser.cpp | 24 +++++++++++-------- mlir/test/AffineOps/ops.mlir | 14 +++++------ mlir/test/GPU/invalid.mlir | 33 +++++++++++--------------- mlir/test/GPU/ops.mlir | 39 ++++++++++++++++--------------- mlir/test/IR/invalid.mlir | 33 +++++++++++++++----------- mlir/test/IR/parser.mlir | 17 +++++++++++--- mlir/test/Transforms/cse.mlir | 4 ++-- 10 files changed, 120 insertions(+), 97 deletions(-) diff --git a/mlir/bindings/python/test/test_py2and3.py b/mlir/bindings/python/test/test_py2and3.py index 72197d9..a004640 100644 --- a/mlir/bindings/python/test/test_py2and3.py +++ b/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( - ' "affine.for"() {lower_bound: () -> (0), step: 1 : index, upper_bound: () -> (5)} : () -> () {\n', - code) + self.assertIn(' "affine.for"() ( {\n', code) self.assertIn(" ^bb1(%i0: index):", code) - self.assertIn( - ' "affine.for"() {lower_bound: () -> (1), step: 3 : index, upper_bound: () -> (15)} : () -> () {\n', - code) + self.assertIn(' "affine.for"() ( {\n', code) self.assertIn(" ^bb2(%i1: index):", code) - self.assertIn( - ' "affine.for"() {lower_bound: () -> (2), step: 5 : index, upper_bound: () -> (25)} : () -> () {\n', - code) + self.assertIn(' "affine.for"() ( {\n', code) self.assertIn(" ^bb3(%i2: index):", code) - self.assertIn( - ' "affine.for"() {lower_bound: () -> (3), step: 7 : index, upper_bound: () -> (35)} : () -> () {\n', - code) + self.assertIn(' "affine.for"() ( {\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) diff --git a/mlir/g3doc/LangRef.md b/mlir/g3doc/LangRef.md index e261a84..a8f62ff 100644 --- a/mlir/g3doc/LangRef.md +++ b/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. diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp index a86821ff..d97b2fc 100644 --- a/mlir/lib/IR/AsmPrinter.cpp +++ b/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 ®ion) { + 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 ®ion : op->getRegions()) - printRegion(region, /*printEntryBlockArgs=*/true, - /*printBlockTerminators=*/true); } void FunctionPrinter::printSuccessorAndUseList(Operation *term, diff --git a/mlir/lib/Parser/Parser.cpp b/mlir/lib/Parser/Parser.cpp index 5d6dad2..9ba043b 100644 --- a/mlir/lib/Parser/Parser.cpp +++ b/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); } diff --git a/mlir/test/AffineOps/ops.mlir b/mlir/test/AffineOps/ops.mlir index 92581ff..7363a39 100644 --- a/mlir/test/AffineOps/ops.mlir +++ b/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"() : () -> () } diff --git a/mlir/test/GPU/invalid.mlir b/mlir/test/GPU/invalid.mlir index 336b0dc..217a656 100644 --- a/mlir/test/GPU/invalid.mlir +++ b/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) - : (index, index, index, index, index) -> () { + "gpu.launch"(%sz, %sz, %sz, %sz, %sz) ({ 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) - : (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): 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) - : (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, @@ -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) - : (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, @@ -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) - : (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): - "region"() : () -> () { - "region"() : () -> () { + "region"() ({ + "region"() ({ // expected-error@+1 {{using value defined outside the region}} "use"(%sz) : (index) -> () - } - } - } + }) : () -> () + }) : () -> () + }) : (index, index, index, index, index, index) -> () return } diff --git a/mlir/test/GPU/ops.mlir b/mlir/test/GPU/ops.mlir index 77dcfee..9d094ef 100644 --- a/mlir/test/GPU/ops.mlir +++ b/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) -> () { - "gpu.launch"(%sz, %sz, %sz, %sz, %sz, %sz) - : (index, index, index, index, index, index) -> () { +// CHECK: "gpu.launch"(%arg0, %arg0, %arg0, %arg0, %arg0, %arg0) +// CHECK-SAME: { + "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) { func @args(%blk : index, %thrd : index, %float : f32, %data : memref) { -// CHECK: "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3) : (index, index, index, index, index, index, f32, memref) -> () { - "gpu.launch"(%blk, %blk, %blk, %thrd, %thrd, %thrd, %float, %data) - : (index, index, index, index, index, index, f32, memref) -> () { +// CHECK: "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3) +// CHECK-SAME: { + "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): return - } +// CHECK: (index, index, index, index, index, index, f32, memref) -> () + }) : (index, index, index, index, index, index, f32, memref) -> () 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) { -// CHECK: "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3) : (index, index, index, index, index, index, f32, memref) -> () { - "gpu.launch"(%blk, %blk, %blk, %thrd, %thrd, %thrd, %float, %data) - : (index, index, index, index, index, index, f32, memref) -> () { +// CHECK: "gpu.launch"(%arg0, %arg0, %arg0, %arg1, %arg1, %arg1, %arg2, %arg3) +// CHECK-SAME: { + "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) ^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 () return - } + }) : (index, index, index, index, index, index, f32, memref) -> () return } @@ -52,20 +54,19 @@ func @passing_values(%blk : index, %thrd : index, %float : f32, %data : memref () { + "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): - "region"() : () -> () { + "region"() ({ // CHECK: %0 = "produce"() %val = "produce"() : () -> (index) - "region"() : () -> () { + "region"() ({ // CHECK: "use"(%0) "use"(%val) : (index) -> () - } - } - } + }) : () -> () + }) : () -> () + }) : (index, index, index, index, index, index) -> () return } diff --git a/mlir/test/IR/invalid.mlir b/mlir/test/IR/invalid.mlir index fe4f7d5..efb5217 100644 --- a/mlir/test/IR/invalid.mlir +++ b/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 () - "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) diff --git a/mlir/test/IR/parser.mlir b/mlir/test/IR/parser.mlir index e56ad97..a565c3b 100644 --- a/mlir/test/IR/parser.mlir +++ b/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 } diff --git a/mlir/test/Transforms/cse.mlir b/mlir/test/Transforms/cse.mlir index 617bd80..b0ba7b9 100644 --- a/mlir/test/Transforms/cse.mlir +++ b/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 } -- 2.7.4