Add verifier for gpu.alloc op

[akshaybaviskar]

Summary

Add verifier for gpu.alloc op to verify if the dimension operand counts and symbol operand counts are same as their memref counterparts.

[bondhugula]

LGTM - thanks. Commit summary is empty. (You can just copy-paste the commit title into the summary here.)

[mehdi_amini]

LGTM - thanks. Commit summary is empty. (You can just copy-paste the commit title into the summary here.)

In git that means just repeating twice the same line in the commit message, am I missing something?

I rather have a description of what the verified is implementing in the description.

[bondhugula]

In D117427#3247168, @mehdi_amini wrote:

LGTM - thanks. Commit summary is empty. (You can just copy-paste the commit title into the summary here.)

In git that means just repeating twice the same line in the commit message, am I missing something?

I rather have a description of what the verified is implementing in the description.

Yes, that would be better, but in some cases, the title itself is a valid summary. Structurally, I think it isn't good to leave the summary empty (for whatever automation may be done in the future on git commits for example) -- the title and summary are distinct things. Duplicating the title into the summary itself is better than no summary I think even if it's adding zero information.

[mehdi_amini]

I’ve never seen a title duplicated in a git commit message, do you have any reference recommending this?

[akshaybaviskar]

In D117427#3247406, @mehdi_amini wrote:

I’ve never seen a title duplicated in a git commit message, do you have any reference recommending this?

Hello Mehdi,

I have updated the commit summary, could you please check if it is okay?

Thanks,
Akshay

[mehdi_amini]

Yes, LG, thanks :)

[akshaybaviskar]

Add verifier for gpu.alloc op to verify if the dimension operand counts and symbol operand counts are same as their memref counterparts.

[herhut]

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

[akshaybaviskar]

In D117427#3248075, @herhut wrote:

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

[akshaybaviskar]

In D117427#3250053, @akshaybaviskar wrote:

In D117427#3248075, @herhut wrote:

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

Hello @herhut,

Could you please reply to this?

Thanks,
Akshay

[bondhugula]

In D117427#3248075, @herhut wrote:

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

It's not clear to me too where this should be exposed from. Also, we can't really reuse everything since memref.alloc/alloca are single-result ops while this one has two results. So things would still spill over unfortunately at both places.

[akshaybaviskar]

Add verifier for gpu.alloc op

[akshaybaviskar]

Add verifier for gpu.alloc op

[herhut]

In D117427#3254758, @akshaybaviskar wrote:

In D117427#3250053, @akshaybaviskar wrote:

In D117427#3248075, @herhut wrote:

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

Hello @herhut,

Could you please reply to this?

Thanks,
Akshay

This dropped off my review queue somehow. You are right, reuse seems difficult and might not be worth it after all.

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp

1177

The more common pattern is to make this a method AllocOp::verify and then have let hasVerifier = 1; in the td file.

[bondhugula]

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp

1177

Looks like I landed this before I saw this comment. @akshaybaviskar could you please address this in a follow-up NFC revision?

[ftynse]

Note that while this was being iterated upon, the verifier field that this uses has been deprecated. Please send a follow-up updating it. https://discourse.llvm.org/t/psa-ods-op-verifier-field-is-being-removed/59714

mlir/include/mlir/Dialect/GPU/GPUOps.td

let extraClassDeclaration = [{
     MemRefType getType() { return memref().getType().cast<MemRefType>(); }
   }];
 
   let assemblyFormat = [{
     custom<AsyncDependencies>(type($asyncToken), $asyncDependencies) ` `
     `(` $dynamicSizes `)` (`` `[` $symbolOperands^ `]`)? attr-dict `:` type($memref)
   }];
 
+  let verifier = [{ return ::verify(*this); }];
   let hasCanonicalizer = 1;
 }
 
 def GPU_DeallocOp : GPU_Op<"dealloc", [GPU_AsyncOpInterface]> {
 
   let summary = "GPU memory deallocation operation";
 
   let description = [{

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp

 LogicalResult MemsetOp::fold(ArrayRef<Attribute> operands,
                              SmallVectorImpl<::mlir::OpFoldResult> &results) {
   return foldMemRefCast(*this);
 }
 
 //===----------------------------------------------------------------------===//
 // GPU_AllocOp
 //===----------------------------------------------------------------------===//
+
+static LogicalResult verify(AllocOp op) {
+  auto memRefType = op.memref().getType().cast<MemRefType>();
+
+  if (static_cast<int64_t>(op.dynamicSizes().size()) !=
+      memRefType.getNumDynamicDims())
+    return op.emitOpError("dimension operand count does not equal memref "
+                          "dynamic dimension count");
+
+  unsigned numSymbols = 0;
+  if (!memRefType.getLayout().isIdentity())
+    numSymbols = memRefType.getLayout().getAffineMap().getNumSymbols();
+  if (op.symbolOperands().size() != numSymbols)
+    return op.emitOpError("symbol operand count does not equal memref symbol "
+                          "count");
+
+  return success();
+}
+
 namespace {
 
 /// Folding of memref.dim(gpu.alloc(%size), %idx) -> %size similar to
 /// `memref::AllocOp`.
 struct SimplifyDimOfAllocOp : public OpRewritePattern<memref::DimOp> {
   using OpRewritePattern<memref::DimOp>::OpRewritePattern;
 
   LogicalResult matchAndRewrite(memref::DimOp dimOp,

mlir/test/Dialect/GPU/invalid.mlir

 func @async_cp_num_dst_stride(
   %dst : memref<200x100xf32, affine_map<(d0, d1) -> (200*d0 + 2*d1)>, 3>,
   %src : memref<200x100xf32>,
   %i : index) -> () {
   // expected-error @+1 {{destination memref most minor dim must have unit stride}}
   gpu.device_async_copy %src[%i, %i], %dst[%i, %i], 16 :
     memref<200x100xf32> to memref<200x100xf32, affine_map<(d0, d1) -> (200*d0 + 2*d1)>, 3>
   return
-}
+}
+
+// -----
+
+// Number of symbol operand count less than memref symbol count.
+func @alloc() {
+   // expected-error@+1 {{symbol operand count does not equal memref symbol count}}
+   %1 = gpu.alloc() : memref<2x4xf32, affine_map<(d0, d1)[s0] -> ((d0 + s0), d1)>, 1>
+   return
+}
+
+// -----
+
+// Number of symbol operand count greater than memref symbol count.
+func @alloc() {
+   %0 = arith.constant 7 : index
+   // expected-error@+1 {{symbol operand count does not equal memref symbol count}}
+   %1 = gpu.alloc()[%0] : memref<2x4xf32, 1>
+   return
+}
+
+// -----
+
+// Number of dynamic dimension operand count greater than memref dynamic dimension count.
+func @alloc() {
+   %0 = arith.constant 7 : index
+   // expected-error@+1 {{dimension operand count does not equal memref dynamic dimension count}}
+   %1 = gpu.alloc(%0, %0) : memref<2x?xf32, 1>
+   return
+}
+
+// -----
+
+// Number of dynamic dimension operand count less than memref dynamic dimension count.
+func @alloc() {
+   %0 = arith.constant 7 : index
+   // expected-error@+1 {{dimension operand count does not equal memref dynamic dimension count}}
+   %1 = gpu.alloc(%0) : memref<2x?x?xf32, 1>
+   return
+}