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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse 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:
> 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.


================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:817
+
+  // Fully specified by traits.
+  let verifier = ?;
----------------
This can be omitted.


================
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,
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1002
+              "Value memref, ValueRange indices", [{
+      auto permMap = AffineMap::getMinorIdentityMap(memref.getType()
+          .cast<MemRefType>().getRank(), /*results=*/1, builder.getContext());
----------------
This looks complex enough to deserve being in a .cpp file instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658