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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
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:
> nicolasvasilache wrote:
> > 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 # "..." 
> > ```
> > 
> > ?
> Something like that should work! However, tablegen complains about `extraClassDeclarationBase` not being defined. I can file a bugzilla for this. I wouldn't like to go into extending ODS in this patch. I could keep `getVectorType` in the base class for now and point to bugzilla.
Also note that part of the base `extraClassDeclaration`, if not all, will go to the interfaces for affine loads/stores (next patch).


================
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
----------------
nicolasvasilache wrote:
> 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`
> 
No strong opinion here but the current combination in `vector.transfer_*` seems ok to me. In any case, if we want to change it, probably better to do it in a separate patch since we would have to change vector tests, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658