[PATCH] D73295: [mlir] [VectorOps] Rewriting of vector.extract/insert_slices to other vector ops

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added a comment.

I really like how this has evolved from the original point, it is almost good to go on my end.



================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:774
+      SmallVector<int64_t, 4> sliceSizes(rank);
+      for (int64_t r = 0; r < rank; ++r) {
+        sliceSizes[r] = std::min(sizes[r], shape[r] - elementOffsets[r]);
----------------
drop trivial braces plz, this is the MLIR style.


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:800
+
+void mlir::vector::populateVectorSlicesTransformationPatterns(
+    OwningRewritePatternList &patterns, MLIRContext *context) {
----------------
So I really like what you're doing re exposing and classifying patterns by **intention**, other places in the codebase should also do that and document it: "this set of patterns is useful for X"

Now, the selection of patterns you chose to add is a bit trickier IMO and I think we should:
1. name this `populateVectorSlicesLoweringPatterns` because it lowers **out of** these ops. Since it does not lower the type it is not a conversion so `LoweringPatterns` seems an appropriate name.
2. insert all the extra patterns (all the necessary tuple stuff) that ensure the Insert/ExtractSlices indeed go away, otherwise it will be surprising that the VectorSlicesLoweringPatterns are not enough to lower the vector slices.
3. explicitly list at the API doc level the patterns that are included (including the ) so people can easily look them up.

After we have enough of those, we will end up with pattern collections that implement **behaviors**.
This will have a granularity somewhere in between (1) individual patterns and (2) full transformations.
I expect this to be very powerful and independently testable like you do.

I am particularly sensitive to this in light of https://reviews.llvm.org/D73145 in which I could not break the phase ordering/dependence for now.

@rriddle  what's your take on this?
Do you see a need / opportunity to have core infra support for collections of patterns and tests?

Side note: 
So far I have shelved the debugging of why  https://reviews.llvm.org/D73145 does not work with fused patterns but we need to resolve it. 


================
Comment at: mlir/test/lib/Transforms/TestVectorTransforms.cpp:41
+    auto *context = &getContext();
+    populateWithGenerated(context, &patterns);
+    populateVectorSlicesTransformationPatterns(patterns, context);
----------------
Re pattern selection, it would be greate that `populateVectorSlicesLoweringPatterns` has everything it needs to make the test pass.
So concretely, this line should go away (and we should hunt other opportunities to improving other `populateXXX` methods by following your model). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73295