[PATCH] D79658: [mlir][Affine] Introduce affine.vector_load and affine.vector_store

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

> This overall looks great. Please also update the commit title for vload/vstore -> vector_load/store. (since arc won't do it by itself as you know.)

Thanks! Yes, I'll clean up the message before committing to remove irrelevant information.



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:826
+    elemental type, supplied as its second operand.
+    The index for each memref dimension is an affine expression of loop
+    induction variables and symbols. These indices determine the write starting
----------------
bondhugula wrote:
> Nit: "... affine function of ... " or "... affine expression in ..."
I think  'expression' is referring to the `AffineExpr` class here (note that this is copy-pasted from AffineStoreOp). "affine expression _in_ loop induction variables and symbols" sounds odd to me but I can be wrong. Is that what you want?


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1002
+              "Value memref, ValueRange indices", [{
+      auto permMap = AffineMap::getMinorIdentityMap(memref.getType()
+          .cast<MemRefType>().getRank(), /*results=*/1, builder.getContext());
----------------
bondhugula wrote:
> ftynse wrote:
> > This looks complex enough to deserve being in a .cpp file instead.
> Was a build method moved into AffineOps.cpp? I don't see anything there. 
It was moved to VectorOps.cpp since this is VectorOps.td


================
Comment at: mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp:579
+    auto permMap = AffineMap::getMinorIdentityMap(op.getMemRefType().getRank(),
+                                                  1, rewriter.getContext());
+    Type elemType = op.getVectorType().getElementType();
----------------
bondhugula wrote:
> Argument comment for 1 (i.e., /*numResults=*/1). I think we should also improve the doc comment for getMinorIdentityMap. It doesn't say what 'results' is, and it should be numResults.
We can actually remove all of this and use the new build with default permutation and padding.


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2570
+  p << " : " << op.getMemRefType() << ", " << op.getValueToStore().getType();
+}
+
----------------
bondhugula wrote:
> We are missing a check to verify that this op's map is indeed a permutation map. Is vector.read/write_transfer also missing it? If yes, please add a TODO for it on AffineVector/Load/Store Op's verifiers. In future, I assume you plan to support strides as well - so that could be marked as a TODO as well. 
The current map in affine.vector_load/store is not a permutation map (at least a permutation map in the way is defined in vector.transfer_load/store), right? IIUC, the one in affine.vector_load/store is the same as in affine.load/store, i.e., it's just for indexing (computing the start position of the read/write), not for permuting the slice we are reading/writing from/to memory. I think we would need a separate map for that in affine, if we are interested in modeling that.

Added TODOs for both things to the doc in the td file but, please, let me know if I'm wrong.


================
Comment at: mlir/test/Dialect/Affine/load-store.mlir:257
+  }
+  return
+}
----------------
bondhugula wrote:
> We could add an extra negative test case to test/Dialect/Affine/invalid.mlir where you use a different element type for the vector and for the memref, and check for errors. 
> `affine.vector_load %A[%i] : memref<100xvector<8xf32>>, vector<16xf32>`
> `affine.vector_store %v, %A : memref<100xf32>, vector<8xf64>`
> `affine.vector_load %A[%i] : memref<100xvector<8xf32>>, vector<8xf32>`
> 
> All of these should fail verification. Very likely that users will try these. :)
Done. I added specific verifiers since this was not covered by the auto-generated ones. This required some refactoring of the scalar verifiers since I didn't want to have the same code replicated 4 times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658