[PATCH] D80098: [mlir][Vector] Add an optional "masked" boolean array attribute to vector transfer operations

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1124
+              "ArrayRef<bool> maybeMasked = {}">,
+    // Builder that sets permutation map to 'getMinorIdentityMap'.
+    OpBuilder<"OpBuilder &builder, OperationState &result, Value vector, "
----------------
The comment looks wrong given that the builder takes a `permutationMap`


================
Comment at: mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp:773
 
 template <>
+LogicalResult replaceTransferOpWithLoadOrStore<TransferReadOp>(
----------------
I wonder if we actually need template specialization here, or just overloading the function on the operation type would have sufficed?


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1305
+  if (optionalMasked) {
+    auto maskedArrayAttr = optionalMasked.cast<ArrayAttr>();
+    if (permutationMap.getNumResults() !=
----------------
It's already an ArrayAttr, no need to cast


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1308
+        static_cast<int64_t>(maskedArrayAttr.size()))
+      return op->emitOpError("requires optional masked attr of same rank as "
+                             "permutation_map results: ")
----------------
Nit: "requires optional" sounds weird to me. "expects the optional 'masked' attr to be of <...> rank when present"?


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1357
+  if (auto maybeMasked = op.masked()) {
+    if (auto masked = maybeMasked->template dyn_cast<ArrayAttr>()) {
+      for (auto attr : masked) {
----------------
Isn't it already an ArrayAttr? In any case, I think you should just `cast` because the verifier ensures it's an array attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80098