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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Just a nit but the one letter difference between `affine.vstore` and `affine.store` does not seem ideal to me from a readability point of view. Also one-letter abbreviation isn't great in general, "v" could be for "volatile" for example.
What about `affine.vector_store` (or `affine.vec_store`)?



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:811
+    ```mlir
+    %1 = affine.load %0[%i0 + symbol(%n), %i1 + symbol(%m)] : memref<100x100xf32>, vector<4xf32>
+    ```
----------------
Typo: `affine.load` -> `affine.vload`


================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:825
+    The "affine.vstore" is the vector counterpart of
+    [affine.store](#affinestore-affinestoreop). It performs a blocking write
+    from a [vector](../LangRef.md#vector-type), supplied as its first operand,
----------------
Do we have another term than "blocking"? It can be ambiguous with respect "synchronous" for example, and it isn't clear to me in general.
Is this "store a vector to a contiguous memory segment"?



================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:998
+  let builders = [
+    // Builder that sets permunation map and padding to 'getMinorIdentityMap'
+    // and zero, respectively, by default.
----------------
Typo `permunation`


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1077
+  let builders = [
+    // Builder that sets permunation map and padding to 'getMinorIdentityMap'
+    // by default.
----------------
Typo `permunation`


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2491
+
+ParseResult parseAffineVLoadOp(OpAsmParser &parser, OperationState &result) {
+  auto &builder = parser.getBuilder();
----------------
nicolasvasilache wrote:
> Not for this revision but generally, should printer/parser based on `parseAffineMapOfSSAIds` have a special syntax in the declarative assembly, e.g. "$affine_indexing" ?
I thought the same! Can you file a bug for this?


================
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>
+  }
----------------
Isn't this reading out-of-bound?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658