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

[Aart Bik via Phabricator via llvm-commits]

aartbik added inline comments.


================
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]);
----------------
nicolasvasilache wrote:
> drop trivial braces plz, this is the MLIR style.
you would think I knew that by now, but old habits die hard....


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:800
+
+void mlir::vector::populateVectorSlicesTransformationPatterns(
+    OwningRewritePatternList &patterns, MLIRContext *context) {
----------------
nicolasvasilache wrote:
> 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. 
Done all (renamed and added doc), except that we don't need any specific tuple stuff anymore! Dead tuples are removed by DCE (in the greedy rewriter at least) while get-tuples-on-tuples are folded away automatically as well!


================
Comment at: mlir/test/lib/Transforms/TestVectorTransforms.cpp:41
+    auto *context = &getContext();
+    populateWithGenerated(context, &patterns);
+    populateVectorSlicesTransformationPatterns(patterns, context);
----------------
nicolasvasilache wrote:
> 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). 
It has. I simply overlooked this line. It is not needed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73295