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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe marked 3 inline comments as done.
dcaballe added inline comments.


================
Comment at: mlir/include/mlir/Conversion/AffineToVector/AffineToVector.h:1
+//===- AffineToVector.h - Convert Affine to Vector dialect ------*- C++ -*-===//
+//
----------------
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.


================
Comment at: mlir/lib/Conversion/AffineToVector/AffineToVector.cpp:55
+        op, op.getVectorType(), op.getMemRef(), *resultOperands,
+        AffineMapAttr::get(permMap), padding);
+    return success();
----------------
bondhugula wrote:
> dcaballe wrote:
> > I would be great if we could have a builder that sets `getMinorIdentityMap` as the default value of the permutation map for those cases where no permutation is needed.
> +1. 
Ok, I'll address this separately


================
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:
> 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...".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79658