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

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added inline comments.


================
Comment at: mlir/include/mlir/Conversion/AffineToVector/AffineToVector.h:1
+//===- AffineToVector.h - Convert Affine to Vector dialect ------*- C++ -*-===//
+//
----------------
dcaballe wrote:
> bondhugula wrote:
> > Instead of another conversion pass for this, we could just do this as part of -lower-affine? Because lower-affine already converts to scf + std, and vector is already at the same "level" of abstraction as std, so, -lower-affine could as well just convert to scf + std + vector. 
> Ok! I did it this way for composability reasons since I need the vector part but not necessarily the scf + std. I'll move it to `-lower-affine` but leave the `populateAffineToVectorConversionPatterns` around so that I can create my own lowering pass locally.
Yes, that sounds good. You don't need a separate pass. Curious: why do you need just the conversion to vector read/write transfer. What will the output give you that you won't get by looking at affine.vload/vstore?


================
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:
> 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.
IMHO, it's more consistent and readable to always find the memref first and the vector type trailing it. The order of operands is secondary - for example, the load doesn't even have a vector type operand. The type list is all custom in the custom form, and so consistency for all memory ops would be good. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658