[mlir][openacc] Switch to assembly format for acc.data

[clementval]

Summary

This patch remove the printer/parser for the acc.data operation since its syntax
fits nicely with the assembly format. It reduces the maintenance for this op.

[kiranktp]

LGTM

[kiranchandramohan]

I understand that this patch is just changing to assembly format but #just-asking, the following clauses are missing.

if( condition )
deviceptr( var-list )
default( none | present )

mlir/test/Dialect/OpenACC/ops.mlir

124

Is this separation of variables and types due to using assembly format? The previous style might be better for longer heterogeneous lists.

[clementval]

In D88330#2296396, @kiranchandramohan wrote:

I understand that this patch is just changing to assembly format but #just-asking, the following clauses are missing.

if( condition )
deviceptr( var-list )
default( none | present )

This is part of a patch series. Look at D88331.

[clementval]

mlir/test/Dialect/OpenACC/ops.mlir

124

Yeah this is because of the assembly format. I'm not aware of a way to keep the previous style without custom parser. All operation I have seen using assembly format have variadic operand set this way.

[kiranchandramohan]

Thanks. LGTM.

[clementval]

In D88330#2296413, @kiranchandramohan wrote:

Thanks. LGTM.

Thanks for the review. If you have time can you have a look at D88326 since it is the first patch of the serie.

[clementval]

Rebase

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td

let description = [{
     acc.data present(%a: memref<10x10xf32>, %b: memref<10x10xf32>,
         %c: memref<10xf32>, %d: memref<10xf32>) {
       // data region
     }
     ```
   }];
 
 
-  let arguments = (ins Variadic<AnyType>:$presentOperands,
-                       Variadic<AnyType>:$copyOperands,
+  let arguments = (ins Variadic<AnyType>:$copyOperands,
                        Variadic<AnyType>:$copyinOperands,
                        Variadic<AnyType>:$copyinReadonlyOperands,
                        Variadic<AnyType>:$copyoutOperands,
                        Variadic<AnyType>:$copyoutZeroOperands,
                        Variadic<AnyType>:$createOperands,
                        Variadic<AnyType>:$createZeroOperands,
                        Variadic<AnyType>:$noCreateOperands,
+                       Variadic<AnyType>:$presentOperands,
                        Variadic<AnyType>:$attachOperands);
 
   let regions = (region AnyRegion:$region);
 
-  let extraClassDeclaration = [{
-    static StringRef getAttachKeyword() { return "attach"; }
-    static StringRef getCopyinKeyword() { return "copyin"; }
-    static StringRef getCopyinReadonlyKeyword() { return "copyin_readonly"; }
-    static StringRef getCopyKeyword() { return "copy"; }
-    static StringRef getCopyoutKeyword() { return "copyout"; }
-    static StringRef getCopyoutZeroKeyword() { return "copyout_zero"; }
-    static StringRef getCreateKeyword() { return "create"; }
-    static StringRef getCreateZeroKeyword() { return "create_zero"; }
-    static StringRef getNoCreateKeyword() { return "no_create"; }
-    static StringRef getPresentKeyword() { return "present"; }
+  let assemblyFormat = [{
+    ( `copy` `(` $copyOperands^ `:` type($copyOperands) `)` )?
+    ( `copyin` `(` $copyinOperands^ `:` type($copyinOperands) `)` )?
+    ( `copyin_readonly` `(` $copyinReadonlyOperands^ `:`
+        type($copyinReadonlyOperands) `)` )?
+    ( `copyout` `(` $copyoutOperands^ `:` type($copyoutOperands) `)` )?
+    ( `copyout_zero` `(` $copyoutZeroOperands^ `:`
+        type($copyoutZeroOperands) `)` )?
+    ( `create` `(` $createOperands^ `:` type($createOperands) `)` )?
+    ( `create_zero` `(` $createZeroOperands^ `:`
+        type($createZeroOperands) `)` )?
+    ( `no_create` `(` $noCreateOperands^ `:` type($noCreateOperands) `)` )?
+    ( `present` `(` $presentOperands^ `:` type($presentOperands) `)` )?
+    ( `attach` `(` $attachOperands^ `:` type($attachOperands) `)` )?
+    $region attr-dict-with-keyword
   }];
 
   let verifier = ?;
 }
 
 def OpenACC_TerminatorOp : OpenACC_Op<"terminator", [Terminator]> {
   let summary = "Generic terminator for OpenACC regions";
 

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp

static void print(OpAsmPrinter &printer, ParallelOp &op) {
   printer.printRegion(op.region(),
                       /*printEntryBlockArgs=*/false,
                       /*printBlockTerminators=*/true);
   printer.printOptionalAttrDictWithKeyword(
       op.getAttrs(), ParallelOp::getOperandSegmentSizeAttr());
 }
 
 //===----------------------------------------------------------------------===//
-// DataOp
-//===----------------------------------------------------------------------===//
-
-/// Parse acc.data operation
-/// operation := `acc.data` (`present` `(` value-list `)`)?
-///                         (`copy` `(` value-list `)`)?
-///                         (`copyin` `(` value-list `)`)?
-///                         (`copyin_readonly` `(` value-list `)`)?
-///                         (`copyout` `(` value-list `)`)?
-///                         (`copyout_zero` `(` value-list `)`)?
-///                         (`create` `(` value-list `)`)?
-///                         (`create_zero` `(` value-list `)`)?
-///                         (`no_create` `(` value-list `)`)?
-///                         (`attach` `(` value-list `)`)?
-///                         region attr-dict?
-static ParseResult parseDataOp(OpAsmParser &parser, OperationState &result) {
-  Builder &builder = parser.getBuilder();
-  SmallVector<OpAsmParser::OperandType, 2> presentOperands, copyOperands,
-      copyinOperands, copyinReadonlyOperands, copyoutOperands,
-      copyoutZeroOperands, createOperands, createZeroOperands, noCreateOperands,
-      attachOperands;
-  SmallVector<Type, 2> presentOperandTypes, copyOperandTypes,
-      copyinOperandTypes, copyinReadonlyOperandTypes, copyoutOperandTypes,
-      copyoutZeroOperandTypes, createOperandTypes, createZeroOperandTypes,
-      noCreateOperandTypes, attachOperandTypes;
-
-  // present(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getPresentKeyword(),
-                              presentOperands, presentOperandTypes, result)))
-    return failure();
-
-  // copy(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCopyKeyword(), copyOperands,
-                              copyOperandTypes, result)))
-    return failure();
-
-  // copyin(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCopyinKeyword(),
-                              copyinOperands, copyinOperandTypes, result)))
-    return failure();
-
-  // copyin_readonly(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCopyinReadonlyKeyword(),
-                              copyinReadonlyOperands, copyinOperandTypes,
-                              result)))
-    return failure();
-
-  // copyout(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCopyoutKeyword(),
-                              copyoutOperands, copyoutOperandTypes, result)))
-    return failure();
-
-  // copyout_zero(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCopyoutZeroKeyword(),
-                              copyoutZeroOperands, copyoutZeroOperandTypes,
-                              result)))
-    return failure();
-
-  // create(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCreateKeyword(),
-                              createOperands, createOperandTypes, result)))
-    return failure();
-
-  // create_zero(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getCreateZeroKeyword(),
-                              createZeroOperands, createZeroOperandTypes,
-                              result)))
-    return failure();
-
-  // no_create(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getNoCreateKeyword(),
-                              noCreateOperands, noCreateOperandTypes, result)))
-    return failure();
-
-  // attach(value-list)?
-  if (failed(parseOperandList(parser, DataOp::getAttachKeyword(),
-                              attachOperands, attachOperandTypes, result)))
-    return failure();
-
-  // Data op region
-  if (failed(parseRegions<ParallelOp>(parser, result)))
-    return failure();
-
-  result.addAttribute(ParallelOp::getOperandSegmentSizeAttr(),
-                      builder.getI32VectorAttr(
-                          {static_cast<int32_t>(presentOperands.size()),
-                           static_cast<int32_t>(copyOperands.size()),
-                           static_cast<int32_t>(copyinOperands.size()),
-                           static_cast<int32_t>(copyinReadonlyOperands.size()),
-                           static_cast<int32_t>(copyoutOperands.size()),
-                           static_cast<int32_t>(copyoutZeroOperands.size()),
-                           static_cast<int32_t>(createOperands.size()),
-                           static_cast<int32_t>(createZeroOperands.size()),
-                           static_cast<int32_t>(noCreateOperands.size()),
-                           static_cast<int32_t>(attachOperands.size())}));
-
-  // Additional attributes
-  if (failed(parser.parseOptionalAttrDictWithKeyword(result.attributes)))
-    return failure();
-
-  return success();
-}
-
-static void print(OpAsmPrinter &printer, DataOp &op) {
-  printer << DataOp::getOperationName();
-
-  // present(value-list)?
-  printOperandList(op.presentOperands(), DataOp::getPresentKeyword(), printer);
-
-  // copy(value-list)?
-  printOperandList(op.copyOperands(), DataOp::getCopyKeyword(), printer);
-
-  // copyin(value-list)?
-  printOperandList(op.copyinOperands(), DataOp::getCopyinKeyword(), printer);
-
-  // copyin_readonly(value-list)?
-  printOperandList(op.copyinReadonlyOperands(),
-                   DataOp::getCopyinReadonlyKeyword(), printer);
-
-  // copyout(value-list)?
-  printOperandList(op.copyoutOperands(), DataOp::getCopyoutKeyword(), printer);
-
-  // copyout(value-list)?
-  printOperandList(op.copyoutZeroOperands(), DataOp::getCopyoutZeroKeyword(),
-                   printer);
-
-  // create(value-list)?
-  printOperandList(op.createOperands(), DataOp::getCreateKeyword(), printer);
-
-  // create_zero(value-list)?
-  printOperandList(op.createZeroOperands(), DataOp::getCreateZeroKeyword(),
-                   printer);
-
-  // no_create(value-list)?
-  printOperandList(op.noCreateOperands(), DataOp::getNoCreateKeyword(),
-                   printer);
-
-  // attach(value-list)?
-  printOperandList(op.attachOperands(), DataOp::getAttachKeyword(), printer);
-
-  printer.printRegion(op.region(),
-                      /*printEntryBlockArgs=*/false,
-                      /*printBlockTerminators=*/true);
-  printer.printOptionalAttrDictWithKeyword(
-      op.getAttrs(), ParallelOp::getOperandSegmentSizeAttr());
-}
-
-//===----------------------------------------------------------------------===//
 // LoopOp
 //===----------------------------------------------------------------------===//
 
 /// Parse acc.loop operation
 /// operation := `acc.loop`
 ///              (`gang` ( `(` (`num=` value)? (`,` `static=` value `)`)? )? )?
 ///              (`vector` ( `(` value `)` )? )? (`worker` (`(` value `)`)? )?
 ///              (`vector_length` `(` value `)`)?

mlir/test/Dialect/OpenACC/ops.mlir

 
 func @compute3(%a: memref<10x10xf32>, %b: memref<10x10xf32>, %c: memref<10xf32>, %d: memref<10xf32>) -> memref<10xf32> {
   %lb = constant 0 : index
   %st = constant 1 : index
   %c10 = constant 10 : index
   %numGangs = constant 10 : i64
   %numWorkers = constant 10 : i64
 
-  acc.data present(%a: memref<10x10xf32>, %b: memref<10x10xf32>, %c: memref<10xf32>, %d: memref<10xf32>) {
+  acc.data present(%a, %b, %c, %d: memref<10x10xf32>, memref<10x10xf32>, memref<10xf32>, memref<10xf32>) {
     acc.parallel num_gangs(%numGangs: i64) num_workers(%numWorkers: i64) private(%c : memref<10xf32>) {
       acc.loop gang {
         scf.for %x = %lb to %c10 step %st {
           acc.loop worker {
             scf.for %y = %lb to %c10 step %st {
               %axy = load %a[%x, %y] : memref<10x10xf32>
               %bxy = load %b[%x, %y] : memref<10x10xf32>
               %tmp = addf %axy, %bxy : f32

 }
 
 // CHECK:      func @compute3({{.*}}: memref<10x10xf32>, {{.*}}: memref<10x10xf32>, [[ARG2:%.*]]: memref<10xf32>, {{.*}}: memref<10xf32>) -> memref<10xf32> {
 // CHECK-NEXT:   [[C0:%.*]] = constant 0 : index
 // CHECK-NEXT:   [[C1:%.*]] = constant 1 : index
 // CHECK-NEXT:   [[C10:%.*]] = constant 10 : index
 // CHECK-NEXT:   [[NUMGANG:%.*]] = constant 10 : i64
 // CHECK-NEXT:   [[NUMWORKERS:%.*]] = constant 10 : i64
-// CHECK-NEXT:   acc.data present(%{{.*}}: memref<10x10xf32>, %{{.*}}: memref<10x10xf32>, %{{.*}}: memref<10xf32>, %{{.*}}: memref<10xf32>) {
+// CHECK-NEXT:   acc.data present(%{{.*}}, %{{.*}}, %{{.*}}, %{{.*}} : memref<10x10xf32>, memref<10x10xf32>, memref<10xf32>, memref<10xf32>) {
 // CHECK-NEXT:     acc.parallel num_gangs([[NUMGANG]]: i64) num_workers([[NUMWORKERS]]: i64) private([[ARG2]]: memref<10xf32>) {
 // CHECK-NEXT:       acc.loop gang {
 // CHECK-NEXT:         scf.for %{{.*}} = [[C0]] to [[C10]] step [[C1]] {
 // CHECK-NEXT:           acc.loop worker {
 // CHECK-NEXT:             scf.for %{{.*}} = [[C0]] to [[C10]] step [[C1]] {
 // CHECK-NEXT:               %{{.*}} = load %{{.*}}[%{{.*}}, %{{.*}}] : memref<10x10xf32>
 // CHECK-NEXT:               %{{.*}} = load %{{.*}}[%{{.*}}, %{{.*}}] : memref<10x10xf32>
 // CHECK-NEXT:               %{{.*}} = addf %{{.*}}, %{{.*}} : f32

 // CHECK:      acc.parallel {
 // CHECK-NEXT: } attributes {waitAttr}
 // CHECK:      acc.parallel {
 // CHECK-NEXT: } attributes {selfAttr}
 
 // -----
 
 func @testdataop(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) -> () {
-  acc.data present(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data present(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data copy(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data copy(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data copyin(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data copyin(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data copyin_readonly(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data copyin_readonly(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data copyout(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data copyout(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data copyout_zero(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data copyout_zero(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data create(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data create(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data create_zero(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data create_zero(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data no_create(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data no_create(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data attach(%a: memref<10xf32>, %b: memref<10xf32>, %c: memref<10x10xf32>) {
+  acc.data attach(%a, %b, %c : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
   }
-  acc.data present(%a: memref<10xf32>) copyin(%b: memref<10xf32>) copyout(%c: memref<10x10xf32>) {
+  acc.data copyin(%b: memref<10xf32>) copyout(%c: memref<10x10xf32>) present(%a: memref<10xf32>) {
   }
   return
 }
 
 // CHECK:      func @testdataop([[ARGA:%.*]]: memref<10xf32>, [[ARGB:%.*]]: memref<10xf32>, [[ARGC:%.*]]: memref<10x10xf32>) {
-// CHECK:      acc.data present([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data present([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data copy([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data copy([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data copyin([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data copyin([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data copyin_readonly([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data copyin_readonly([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data copyout([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data copyout([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data copyout_zero([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data copyout_zero([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data create([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data create([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data create_zero([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data create_zero([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data no_create([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data no_create([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data attach([[ARGA]]: memref<10xf32>, [[ARGB]]: memref<10xf32>, [[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data attach([[ARGA]], [[ARGB]], [[ARGC]] : memref<10xf32>, memref<10xf32>, memref<10x10xf32>) {
 // CHECK-NEXT: }
-// CHECK:      acc.data present([[ARGA]]: memref<10xf32>) copyin([[ARGB]]: memref<10xf32>) copyout([[ARGC]]: memref<10x10xf32>) {
+// CHECK:      acc.data copyin([[ARGB]] : memref<10xf32>) copyout([[ARGC]] : memref<10x10xf32>) present([[ARGA]] : memref<10xf32>) {
 // CHECK-NEXT: }