[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

257–270

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

673

don't add trailing dots to error messages

676

Nit: terminate sentences with a dot.

683

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

199–201

These changes look irrelevant to the commit.

[clementval]

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

683

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

[ftynse]

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

683

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

199–201

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

[rriddle]

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

481

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

679

Missing returns here.

[ftynse]

mlir/test/Dialect/OpenACC/ops.mlir

199–201

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.

247

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

481

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

mlir/test/Dialect/OpenACC/ops.mlir

199–201

Sure makes sense. I'll update that.

247

Sure I'll use a dummy here as well.

247

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

[clementval]

Rebase + address review comments

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

let extraClassDeclaration = [{
     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"; }
+    bool isAuto() {
+      UnitAttr isAutoAttr = getAttrOfType<UnitAttr>(getAutoAttrName());
+      return static_cast<bool>(isAutoAttr);
+    }
+    bool isIndependent() {
+      UnitAttr isIndependentAttr =
+          getAttrOfType<UnitAttr>(getIndependentAttrName());
+      return static_cast<bool>(isIndependentAttr);
+    }
+    bool isSeq() {
+      UnitAttr isSeqAttr = getAttrOfType<UnitAttr>(getSeqAttrName());
+      return static_cast<bool>(isSeqAttr);
+    }
+    unsigned getExecutionMapping();
   }];
 
-  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 = [{

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

static void print(OpAsmPrinter &printer, DataOp &op) {
   printer.printOptionalAttrDictWithKeyword(
       op.getAttrs(), ParallelOp::getOperandSegmentSizeAttr());
 }
 
 //===----------------------------------------------------------------------===//
 // LoopOp
 //===----------------------------------------------------------------------===//
 
+unsigned LoopOp::getExecutionMapping() {
+  IntegerAttr execMapping =
+      getAttrOfType<IntegerAttr>(getExecutionMappingAttrName());
+  return execMapping ? execMapping.getInt()
+                     : static_cast<unsigned>(acc::OpenACCExecMapping::NONE);
+}
+
 /// 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;
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)))
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.isAuto() && (loopOp.isIndependent() || loopOp.isSeq())) ||
+      (loopOp.isIndependent() && loopOp.isSeq())) {
+    return loopOp.emitError("only one of " + acc::LoopOp::getAutoAttrName() +
+                            ", " + acc::LoopOp::getIndependentAttrName() +
+                            ", " + acc::LoopOp::getSeqAttrName() +
+                            " can be present at the same time.");
+  }
+
+  // Gang, worker and vector are incompatible with seq
+  if (loopOp.isSeq() &&
+      loopOp.getExecutionMapping() != OpenACCExecMapping::NONE) {
+    loopOp.emitError("gang, worker or vector cannot appear with the seq attr.");
+  }
+
+  // Check non-empty body ()
+  if (llvm::hasSingleElement(loopOp.region()) &&
+      llvm::hasSingleElement(loopOp.region().front().getOperations()))
+    loopOp.emitError("expected non-empty body.");
+
+  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
+
+func @loop_seq() {
+  %lb = constant 0 : index
+  %ub = constant 10 : index
+  %st = constant 1 : index
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop gang {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop worker {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop vector {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop gang worker {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop gang vector {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop worker vector {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  // expected-error@+1 {{gang, worker or vector cannot appear with the seq attr.}}
+  acc.loop gang worker vector {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
+  } attributes {seq}
+  return
+}
+
+// ---
+
+func @loop_empty_body() {
+  // expected-error@+1 {{expected non-empty body.}}
+  acc.loop {
+    acc.yield
+  }
+  return
+}
+
+// ---
+
+func @loop_exclusive_attr() {
+  // expected-error@+1 {{only one of auto, independent, seq can be present at the same time.}}
+  acc.loop {
+    acc.yield
+  } attributes {auto, seq}
+  return
+}
+
+// ---

mlir/test/Dialect/OpenACC/ops.mlir

 // 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>) -> () {
+  %lb = constant 0 : index
+  %ub = constant 10 : index
+  %st = constant 1 : index
   %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 {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop gang(num: %gangNum) {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop gang(static: %gangStatic) {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop worker(%workerNum) {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop vector(%vectorLength) {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop gang(num: %gangNum) worker vector {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop gang(num: %gangNum, static: %gangStatic) worker(%workerNum) vector(%vectorLength) {
+    scf.for %x = %lb to %ub step %st {
+    }
+    acc.yield
   }
   acc.loop tile(%tileSize : i64, %tileSize : i64) {
+    scf.for %x = %lb to %ub step %st {
+    }
+    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:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(num: [[GANGNUM]]) {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(static: [[GANGSTATIC]]) {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop worker([[WORKERNUM]]) {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop vector([[VECTORLENGTH]]) {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(num: [[GANGNUM]]) worker vector {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop gang(num: [[GANGNUM]], static: [[GANGSTATIC]]) worker([[WORKERNUM]]) vector([[VECTORLENGTH]]) {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }
 // CHECK-NEXT: acc.loop tile([[TILESIZE]]: i64, [[TILESIZE]]: i64) {
+// CHECK-NEXT:   scf.for %{{.*}} = %{{.*}} to %{{.*}} step %{{.*}} {
+// CHECK-NEXT:   }
+// CHECK-NEXT:   acc.yield
 // CHECK-NEXT: }