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

Tue May 12 13:59:50 PDT 2020

dcaballe added inline comments.

================
Comment at: mlir/include/mlir/Conversion/AffineToVector/AffineToVector.h:1
+//===- AffineToVector.h - Convert Affine to Vector dialect ------*- C++ -*-===//
+//
----------------
bondhugula wrote:
> 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?
I meant that I may want to compose the affine->vector lowering with the lowering to other dialects or a customized scf+std lowering.

================
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 = [{
----------------
ftynse wrote:
> dcaballe wrote:
> > 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).
> > However, tablegen complains about extraClassDeclarationBase not being defined. 
> 
> `let` _redefines_ a variable, `<typename>` defines a new one. Try `code extraClassDeclarationBase = [{...}];` instead.
Awesome! It works, thanks!

================
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,
----------------
ftynse wrote:
> dcaballe wrote:
> > mehdi_amini wrote:
> > > 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"?
> > > 
> > This doc is a copy-paste mix of `AffineStoreOp` doc and `VectorTransferWriteOp` doc, including the "blocking" terminology. I decided to do that for consistent with the vector dialect. I can replace "blocking write" by something along the lines of "store to a vector to a contiguous memory segment" in both dialects if that is clearer. We may have to revisit this doc anyways if/when we introduce strided or gather/scatter accesses.
> I think the "blocking" terminology in vector transfers indicates that it's not a DMA operation and it will block execution until the result is ready. It doesn't make sense for load/store.
I removed "blocking" and rephrase it a bit, describing the read/write starting position and the shape of the slice being read/written. Please, let me know if you have more comments. We can iterate on this.

================
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
----------------
bondhugula wrote:
> 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. 
Ok, I changed that for `affine.vector_load` and `affine.vector_store`. We can change `vector.transfer_*` separately.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658