[PATCH] D79658: [mlir][Affine] Introduce affine.vload and affine.vstore
Uday Bondhugula via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 12 23:57:43 PDT 2020
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
More information about the llvm-commits
mailing list