[PATCH] D79658: [mlir][Affine] Introduce affine.vector_load and affine.vector_store

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula accepted this revision.
bondhugula marked 2 inline comments as done.
bondhugula added a comment.

Looks good. All your examples/test cases are with 1-d vectors, but the op documentation isn't restricted. You may just want to either specifically add that the vector could be multi-dimensional or include one test case in load-store.mlir (or example in the op doc). I assume such multi-dimensional affine vector load/stores correctly lower to vector read/write transfer as is with what you have.



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineOps.td:826
+    elemental type, supplied as its second operand.
+    The index for each memref dimension is an affine expression of loop
+    induction variables and symbols. These indices determine the write starting
----------------
dcaballe wrote:
> bondhugula wrote:
> > Nit: "... affine function of ... " or "... affine expression in ..."
> I think  'expression' is referring to the `AffineExpr` class here (note that this is copy-pasted from AffineStoreOp). "affine expression _in_ loop induction variables and symbols" sounds odd to me but I can be wrong. Is that what you want?
Never mind - this is too minor. Sounds good as is. 


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2570
+  p << " : " << op.getMemRefType() << ", " << op.getValueToStore().getType();
+}
+
----------------
dcaballe wrote:
> bondhugula wrote:
> > We are missing a check to verify that this op's map is indeed a permutation map. Is vector.read/write_transfer also missing it? If yes, please add a TODO for it on AffineVector/Load/Store Op's verifiers. In future, I assume you plan to support strides as well - so that could be marked as a TODO as well. 
> The current map in affine.vector_load/store is not a permutation map (at least a permutation map in the way is defined in vector.transfer_load/store), right? IIUC, the one in affine.vector_load/store is the same as in affine.load/store, i.e., it's just for indexing (computing the start position of the read/write), not for permuting the slice we are reading/writing from/to memory. I think we would need a separate map for that in affine, if we are interested in modeling that.
> 
> Added TODOs for both things to the doc in the td file but, please, let me know if I'm wrong.
Oh, I see. Looks like I got confused. There is no permutation map here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658