[PATCH] D146724: [mlir][MemRef] Add patterns to extract address computations

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Dialect/MemRef/Transforms/Transforms.h:35
+/// ```
+void populateExtractAddressComputationsPatterns(RewritePatternSet &patterns);
+
----------------
qcolombet wrote:
> nicolasvasilache wrote:
> > also move the other populates here ?
> I thought we would do that in an NFC commit.
> Which I'll do after that one land.
sgtm!


================
Comment at: mlir/lib/Dialect/MemRef/Transforms/ExtractAddressComputations.cpp:223
+template <typename LoadStoreLikeOp,
+          FailureOr<Value> (*getFailureOrSrcMemRef)(LoadStoreLikeOp),
+          LoadStoreLikeOp (*rebuildOpFromAddressAndIndices)(
----------------
qcolombet wrote:
> qcolombet wrote:
> > nicolasvasilache wrote:
> > > llvm::function_ref instad of C style funptrs ?
> > Good point, I always forgot about these!
> > (I start with std::function, then remember that templates don't like them and resort to plain C. I need to update my mental model :)).
> Alright looks like we can't use them either. They suffer the same issue as `std::function`:
> ```
> error: non-type template parameters of class type only available with ‘-std=c++20’ or ‘-std=gnu++20’
> ```
> 
> Am I missing something?
oh well .. 


================
Comment at: mlir/test/Dialect/MemRef/extract-address-computations.mlir:287
+  %vcf0 = arith.constant dense<0.000000e+00> : vector<4x2xf16>
+  %res = vector.transfer_write %vcf0, %base[%offset0, %offset1, %offset2] { permutation_map = affine_map<(d0,d1,d2) -> (d2,d0)> } : vector<4x2xf16>, tensor<?x?x?xf16>
+  return %res : tensor<?x?x?xf16>
----------------
qcolombet wrote:
> qcolombet wrote:
> > nicolasvasilache wrote:
> > > some examples with strides please that mirror my previous comment
> > 👍
> Only did one for `vector.transfer_write` (see `test_transfer_write_op_with_strides`).
> Let me know if you want some for the others.
that's great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146724