[PATCH] D79658: [mlir][Affine] Introduce affine.vload and affine.vstore
Mehdi AMINI via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 11 21:03:47 PDT 2020
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
More information about the llvm-commits
mailing list