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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.

Great, thanks @dcaballe !



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:817
+  // extraClassDeclaration. Is there a way to add more declarations without
+  // overriding AffineLoadOpBase's.
+  // let extraClassDeclaration = [{
----------------
dcaballe wrote:
> Not sure if it's feasible at all in tablegen but it would be great if we could make "code" variables (`extraClassDeclaration`, `builders`, etc.) "additive" in a hierarchy instead of overriding the variable with the value from the subclass.
In base:
```
 let extraClassDeclarationBase = " ... " 
```

In derived:
```
 let extraClassDeclaration =  extraClassDeclarationBase # "..." 
```

?


================
Comment at: mlir/lib/Conversion/AffineToVector/AffineToVector.cpp:55
+        op, op.getVectorType(), op.getMemRef(), *resultOperands,
+        AffineMapAttr::get(permMap), padding);
+    return success();
----------------
dcaballe wrote:
> bondhugula wrote:
> > dcaballe wrote:
> > > I would be great if we could have a builder that sets `getMinorIdentityMap` as the default value of the permutation map for those cases where no permutation is needed.
> > +1. 
> Ok, I'll address this separately
Feel free to also fold it into this revision if you want.


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


================
Comment at: mlir/test/Dialect/Affine/load-store.mlir:228
+      %1 = affine.vload %0[%i0, %i1] : memref<100x100xf32>, vector<8xf32>
+      affine.vstore %1, %0[%i0, %i1] : vector<8xf32>, memref<100x100xf32>
+// CHECK:      %[[buf:.*]] = alloc
----------------
dcaballe wrote:
> bondhugula wrote:
> > Do you want to switch the type order in the type list to keep it consistent with affine.load? Just makes it easier to copy / paste while writing test cases  and a bit consistent. The order of the operands need not be the same as in the colon type list, but it's the semantics "store to a memref of type ..., a vector value of type ..." that we'd read.
> Not sure about this. I tried to keep it consistent with `vector.transfer_write` so that we don't have to do a different interpretation of a vector store depending on the dialect we are. It could be confusing if we switch the type order since one would initially expect the types to match the order of the operands when the number of operands and types are the same (at least I would :)). The way I read it is more like: "store a vector value of type ... to a memref of type...".
feel free to change this syntax, parser and printer + the vector_transfer syntax to whichever combination of you prefer:
`vector from memref` / `vector to memref` or `memref to vector` / `memref from vector`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658