[PATCH] D80092: [mlir][Vector] Make minor identity permutation map optional in transfer op printing and parsing

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:866
 
+class Vector_TransferOpBase<int dummy> {
+  code extraTransferDeclaration = [{
----------------
You don't need a class unless it's parameterized. Try

```
def Vector_TransferOpUtils {
  code transferDeclaration = [{...}];
}

def Vector_TransferReadOp : ...{
  let extraClassDeclaration = Vector_TransferOpUtils.transferdeclaration # [{...}];
}
```

without classes or inheritance.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:877
+    /// This also handles the case memref<... x vector<...>> -> vector<...> in
+    /// which the rank of tte identity map must take the vector element type
+    /// into account.
----------------
Typo: tte


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1331
+                                                      VectorType vectorType) {
+  return ::impl::getTransferMinorIdentityMap(memRefType, vectorType);
+}
----------------
Can you just put this into the declaration of the function in .td ?


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1337
+  SmallVector<StringRef, 1> elidedAttrs;
+  if (AffineMap::isMinorIdentity(op.permutation_map()))
+    elidedAttrs.push_back(op.getPermutationMapAttrName());
----------------
Should we also check that it is a minor identity of the correct size, i.e. has the expected number of equal dimensions? Or is it impossible for valid ops?


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1371
+  if (!vectorType)
+    return parser.emitError(typesLoc, "vector type required"), failure();
+  auto permutationAttrName = TransferReadOp::getPermutationMapAttrName();
----------------
Please add tests for user-visible error messages


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1462
+  if (!vectorType)
+    return parser.emitError(typesLoc, "vector type required"), failure();
+  MemRefType memRefType = types[1].dyn_cast<MemRefType>();
----------------
parser.emitError(..) is convertible to ParseResult::failure() IIRC


================
Comment at: mlir/test/Conversion/AffineToStandard/lower-affine-to-vector.mlir:3
 
 // CHECK: #[[perm_map:.*]] = affine_map<(d0) -> (d0)>
 // CHECK-LABEL: func @affine_vector_load
----------------
Please drop the capture for the map if it is no longer necessary. Here and below, maybe in other files too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80092/new/

https://reviews.llvm.org/D80092