[PATCH] D79658: [mlir][Affine] Introduce affine.vload and affine.vstore

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula accepted this revision.
bondhugula added a comment.
This revision is now accepted and ready to land.

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.)



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:783
+    The "affine.vector_load" is the vector counterpart of
+    [affine.load](#affineload-operation). It reads a slice from a
+    [MemRef](../LangRef.md#memref-type), supplied as its first operand,
----------------
Nit: I assume you also want strides in the future. You could document somewhere that this slice/vector is currently contiguous along the respective dimensions, but will be extended to strided vectors. 


================
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
----------------
Nit: "... affine function of ... " or "... affine expression in ..."


================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:827
+    The index for each memref dimension is an affine expression of loop
+    induction variables and symbols. These indices determine the write starting
+    position within the memref. The shape of the input vector determines the
----------------
Nit: ... the start position of the write within ...


================
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());
----------------
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. 


================
Comment at: mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp:579
+    auto permMap = AffineMap::getMinorIdentityMap(op.getMemRefType().getRank(),
+                                                  1, rewriter.getContext());
+    Type elemType = op.getVectorType().getElementType();
----------------
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.


================
Comment at: mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp:607
+
+    // Build std.store valueToStore, memref[expandedMap.results].
+    auto permMap = AffineMap::getMinorIdentityMap(op.getMemRefType().getRank(),
----------------
Comment update: std.store -> 


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2570
+  p << " : " << op.getMemRefType() << ", " << op.getValueToStore().getType();
+}
+
----------------
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. 


================
Comment at: mlir/test/Conversion/AffineToStandard/lower-affine-to-vector.mlir:8
+  affine.for %i0 = 0 to 16 {
+    %1 = affine.vload %0[%i0 + symbol(%arg0) + 7] : memref<16xf32>, vector<8xf32>
+  }
----------------
dcaballe wrote:
> mehdi_amini wrote:
> > Isn't this reading out-of-bound?
> It might anyways due to `arg0` being an arbitrary input but I'll make it less obvious :)
-memref-bound-check can catch such out-of-bound errors (but for affine.load/store ops). 


================
Comment at: mlir/test/Dialect/Affine/load-store.mlir:220
+
+// CHECK: [[MAP0:#map[0-9]+]] = affine_map<(d0, d1) -> (d0, d1)>
+
----------------
Nit: MAP0 -> MAP_ID or IDENTITY  for readability.


================
Comment at: mlir/test/Dialect/Affine/load-store.mlir:257
+  }
+  return
+}
----------------
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. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658