[mlir][openacc] Add loop op verifier

[clementval]

Summary

Add a verifier for the loop op in the OpenACC dialect. Check basic restriction
from 2.9 Loop construct from the OpenACC 3.0 specs.

[ftynse]

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

259–272

Tablegen generates accessors for all attributes using their names, with proper value casting, loopAuto(), loopIndependent() and loopSeq() in this case. Please use those instead of reimplementing them.

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

665

don't add trailing dots to error messages

668

Nit: terminate sentences with a dot.

675

This can be expressed as a trait in tablegen: either SingleBlockImplicitTerminator if you also want the terminator kind to be verified and automatically added by the parser, or SizedRegion<1> if you only want single-block regions.

mlir/test/Dialect/OpenACC/ops.mlir

196–198

These changes look irrelevant to the commit.

[clementval]

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

675

I would like to check that there is something else than the terminator.

[ftynse]

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

675

Then use SizedRegion<1> to check for a single block, and then write the proper check for more operations.

[clementval]

Address review comments

mlir/test/Dialect/OpenACC/ops.mlir

196–198

With the new verifier empty body will trigger an error therefore I added smith in.

[rriddle]

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

486

I'm pretty sure you can model this using a DefaultValueAttr in ODS.

671

Missing returns here.

[ftynse]

mlir/test/Dialect/OpenACC/ops.mlir

196–198

I'd just add "some.op"() : () -> (), which is shorter and does not imply there is some semantics associated with SCF fors being present under acc.loop.

236

Please don't tests for irrelevant things such as scf.for syntax. It has absolutely no connection to the acc dialect and just makes it more painful for anybody willing to change scf.for itself.

[clementval]

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

486

Sure, I have update the op to use the DefaultValuedAttr and it works fine.

mlir/test/Dialect/OpenACC/ops.mlir

196–198

Sure makes sense. I'll update that.

236

Sure I'll use a dummy here as well.

236

I updated the patch to use a dummy op here as well.

[clementval]

Rebase + address review comments

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

def OpenACC_TerminatorOp : OpenACC_Op<"terminator", [Terminator]> {
   let assemblyFormat = "attr-dict";
 }
 
 //===----------------------------------------------------------------------===//
 // 2.9 loop Construct
 //===----------------------------------------------------------------------===//
 
 def OpenACC_LoopOp : OpenACC_Op<"loop",
-                        [AttrSizedOperandSegments]> {
+      [AttrSizedOperandSegments,
+       SingleBlockImplicitTerminator<"acc::YieldOp">]> {
   let summary = "loop construct";
 
   let description = [{
     The "acc.loop" operation represents the OpenACC loop construct.
 
     Example:
 
     ```mlir
def OpenACC_LoopOp : OpenACC_Op<"loop",
   }];
 
 
   let arguments = (ins OptionalAttr<I64Attr>:$collapse,
                        Optional<AnyInteger>:$gangNum,
                        Optional<AnyInteger>:$gangStatic,
                        Optional<AnyInteger>:$workerNum,
                        Optional<AnyInteger>:$vectorLength,
-                       UnitAttr:$loopSeq,
-                       UnitAttr:$loopIndependent,
-                       UnitAttr:$loopAuto,
+                       UnitAttr:$seq,
+                       UnitAttr:$independent,
+                       UnitAttr:$auto_,
                        Variadic<AnyInteger>:$tileOperands,
                        Variadic<AnyType>:$privateOperands,
                        OptionalAttr<OpenACC_ReductionOpAttr>:$reductionOp,
-                       Variadic<AnyType>:$reductionOperands);
+                       Variadic<AnyType>:$reductionOperands,
+                       DefaultValuedAttr<I64Attr, "0">:$exec_mapping);
 
   let results = (outs Variadic<AnyType>:$results);
 
   let regions = (region AnyRegion:$region);
 
   let extraClassDeclaration = [{
     static StringRef getCollapseAttrName() { return "collapse"; }
     static StringRef getSeqAttrName() { return "seq"; }
     static StringRef getIndependentAttrName() { return "independent"; }
     static StringRef getAutoAttrName() { return "auto"; }
     static StringRef getExecutionMappingAttrName() { return "exec_mapping"; }
     static StringRef getGangKeyword() { return "gang"; }
     static StringRef getGangNumKeyword() { return "num"; }
     static StringRef getGangStaticKeyword() { return "static"; }
     static StringRef getVectorKeyword() { return "vector"; }
     static StringRef getWorkerKeyword() { return "worker"; }
     static StringRef getTileKeyword() { return "tile"; }
     static StringRef getPrivateKeyword() { return "private"; }
     static StringRef getReductionKeyword() { return "reduction"; }
   }];
 
-  let verifier = ?;
+  let verifier = [{ return ::verifyLoopOp(*this); }];
 }
 
 // Yield operation for the acc.loop and acc.parallel operations.
 def OpenACC_YieldOp : OpenACC_Op<"yield", [Terminator,
     ParentOneOf<["ParallelOp, LoopOp"]>]> {
   let summary = "Acc yield and termination operation";
 
   let description = [{
     `acc.yield` is a special terminator operation for block inside regions in
     acc ops (parallel and loop). It returns values to the immediately enclosing
     acc op.
   }];
 
   let arguments = (ins Variadic<AnyType>:$operands);
 
   let builders = [
     OpBuilder<"OpBuilder &builder, OperationState &result",
               [{ /* nothing to do */ }]>
   ];
 
   let verifier = ?;
 
   let assemblyFormat = "attr-dict ($operands^ `:` type($operands))?";
 }
 
 #endif // OPENACC_OPS
 

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

 
 //===----------------------------------------------------------------------===//
 // LoopOp
 //===----------------------------------------------------------------------===//
 
 /// Parse acc.loop operation
 /// operation := `acc.loop` `gang`? `vector`? `worker`?
 ///                         `private` `(` value-list `)`?
 ///                         `reduction` `(` value-list `)`?
 ///                         region attr-dict?
 static ParseResult parseLoopOp(OpAsmParser &parser, OperationState &result) {
   Builder &builder = parser.getBuilder();
-  unsigned executionMapping = 0;
+  unsigned executionMapping = OpenACCExecMapping::NONE;
   SmallVector<Type, 8> operandTypes;
   SmallVector<OpAsmParser::OperandType, 8> privateOperands, reductionOperands;
   SmallVector<OpAsmParser::OperandType, 8> tileOperands;
   bool hasWorkerNum = false, hasVectorLength = false, hasGangNum = false;
   bool hasGangStatic = false;
   OpAsmParser::OperandType workerNum, vectorLength, gangNum, gangStatic;
   Type intType = builder.getI64Type();
 
if (failed(parseOperandList(parser, LoopOp::getPrivateKeyword(),
                               privateOperands, operandTypes, result)))
     return failure();
 
   // reduction()?
   if (failed(parseOperandList(parser, LoopOp::getReductionKeyword(),
                               reductionOperands, operandTypes, result)))
     return failure();
 
-  if (executionMapping != 0)
+  if (executionMapping != acc::OpenACCExecMapping::NONE)
     result.addAttribute(LoopOp::getExecutionMappingAttrName(),
                         builder.getI64IntegerAttr(executionMapping));
 
   // Parse optional results in case there is a reduce.
   if (parser.parseOptionalArrowTypeList(result.types))
     return failure();
 
   if (failed(parseRegions<LoopOp>(parser, result)))
if (failed(parser.parseOptionalAttrDictWithKeyword(result.attributes)))
     return failure();
 
   return success();
 }
 
 static void print(OpAsmPrinter &printer, LoopOp &op) {
   printer << LoopOp::getOperationName();
 
-  unsigned execMapping =
-      (op.getAttrOfType<IntegerAttr>(LoopOp::getExecutionMappingAttrName()) !=
-       nullptr)
-          ? op.getAttrOfType<IntegerAttr>(LoopOp::getExecutionMappingAttrName())
-                .getInt()
-          : 0;
-
+  unsigned execMapping = op.exec_mapping();
   if (execMapping & OpenACCExecMapping::GANG) {
     printer << " " << LoopOp::getGangKeyword();
     Value gangNum = op.gangNum();
     Value gangStatic = op.gangStatic();
 
     // Print optional gang operands
     if (gangNum || gangStatic) {
       printer << "(";
printer.printRegion(op.region(),
                       /*printEntryBlockArgs=*/false,
                       /*printBlockTerminators=*/true);
 
   printer.printOptionalAttrDictWithKeyword(
       op.getAttrs(), {LoopOp::getExecutionMappingAttrName(),
                       LoopOp::getOperandSegmentSizeAttr()});
 }
 
+static LogicalResult verifyLoopOp(acc::LoopOp loopOp) {
+  // auto, independent and seq attribute are mutually exclusive.
+  if ((loopOp.auto_() && (loopOp.independent() || loopOp.seq())) ||
+      (loopOp.independent() && loopOp.seq())) {
+    loopOp.emitError("only one of " + acc::LoopOp::getAutoAttrName() + ", " +
+                     acc::LoopOp::getIndependentAttrName() + ", " +
+                     acc::LoopOp::getSeqAttrName() +
+                     " can be present at the same time");
+    return failure();
+  }
+
+  // Gang, worker and vector are incompatible with seq.
+  if (loopOp.seq() && loopOp.exec_mapping() != OpenACCExecMapping::NONE) {
+    loopOp.emitError("gang, worker or vector cannot appear with the seq attr");
+    return failure();
+  }
+
+  // Check non-empty body().
+  if (loopOp.region().empty()) {
+    loopOp.emitError("expected non-empty body.");
+    return failure();
+  }
+
+  return success();
+}
+
 #define GET_OP_CLASSES
 #include "mlir/Dialect/OpenACC/OpenACCOps.cpp.inc"

mlir/test/Dialect/OpenACC/invalid.mlir

+// RUN: mlir-opt -split-input-file -verify-diagnostics %s
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop gang {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop worker {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop vector {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop gang worker {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop gang vector {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop worker vector {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{gang, worker or vector cannot appear with the seq attr}}
+acc.loop gang worker vector {
+  "some.op"() : () -> ()
+  acc.yield
+} attributes {seq}
+
+// -----
+
+// expected-error@+1 {{expected non-empty body.}}
+acc.loop {
+}
+
+// -----
+
+// expected-error@+1 {{only one of auto, independent, seq can be present at the same time}}
+acc.loop {
+  acc.yield
+} attributes {auto_, seq}
+
+// -----

mlir/test/Dialect/OpenACC/ops.mlir

-// RUN: mlir-opt %s | FileCheck %s
+// RUN: mlir-opt -allow-unregistered-dialect %s | FileCheck %s
 // Verify the printed output can be parsed.
-// RUN: mlir-opt %s | mlir-opt | FileCheck %s
+// RUN: mlir-opt -allow-unregistered-dialect %s | mlir-opt -allow-unregistered-dialect  | FileCheck %s
 // Verify the generic form can be parsed.
-// RUN: mlir-opt -mlir-print-op-generic %s | mlir-opt | FileCheck %s
+// RUN: mlir-opt -allow-unregistered-dialect -mlir-print-op-generic %s | mlir-opt -allow-unregistered-dialect | FileCheck %s
 
 func @compute1(%A: memref<10x10xf32>, %B: memref<10x10xf32>, %C: memref<10x10xf32>) -> memref<10x10xf32> {
   %c0 = constant 0 : index
   %c10 = constant 10 : index
   %c1 = constant 1 : index
 
   acc.parallel async(%c1) {
     acc.loop gang vector {

 // CHECK-NEXT:       }
 // CHECK-NEXT:       acc.yield
 // CHECK-NEXT:     }
 // CHECK-NEXT:     acc.terminator
 // CHECK-NEXT:   }
 // CHECK-NEXT:   return %{{.*}} : memref<10xf32>
 // CHECK-NEXT: }
 
-func @testop() -> () {
+func @testop(%a: memref<10xf32>) -> () {
   %workerNum = constant 1 : i64
   %vectorLength = constant 128 : i64
   %gangNum = constant 8 : i64
   %gangStatic = constant 2 : i64
   %tileSize = constant 2 : i64
   acc.loop gang worker vector {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop gang(num: %gangNum) {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop gang(static: %gangStatic) {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop worker(%workerNum) {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop vector(%vectorLength) {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop gang(num: %gangNum) worker vector {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop gang(num: %gangNum, static: %gangStatic) worker(%workerNum) vector(%vectorLength) {
+    "some.op"() : () -> ()
+    acc.yield
   }
   acc.loop tile(%tileSize : i64, %tileSize : i64) {
+    "some.op"() : () -> ()
+    acc.yield
   }
   return
 }
 
 // CHECK:      [[WORKERNUM:%.*]] = constant 1 : i64
 // CHECK-NEXT: [[VECTORLENGTH:%.*]] = constant 128 : i64
 // CHECK-NEXT: [[GANGNUM:%.*]] = constant 8 : i64
 // CHECK-NEXT: [[GANGSTATIC:%.*]] = constant 2 : i64
 // CHECK-NEXT: [[TILESIZE:%.*]] = constant 2 : i64
 // CHECK-NEXT: acc.loop gang worker vector {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(num: [[GANGNUM]]) {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(static: [[GANGSTATIC]]) {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop worker([[WORKERNUM]]) {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop vector([[VECTORLENGTH]]) {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(num: [[GANGNUM]]) worker vector {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(num: [[GANGNUM]], static: [[GANGSTATIC]]) worker([[WORKERNUM]]) vector([[VECTORLENGTH]]) {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop tile([[TILESIZE]]: i64, [[TILESIZE]]: i64) {
+// CHECK-NEXT:   "some.op"() : () -> ()
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 
 
 func @testparallelop() -> () {
   %vectorLength = constant 128 : index
   acc.parallel vector_length(%vectorLength) {
   }
   return
 }
 
 // CHECK:      [[VECTORLENGTH:%.*]] = constant 128 : index
 // CHECK-NEXT: acc.parallel vector_length([[VECTORLENGTH]]) {
 // CHECK-NEXT: }