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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:800
+
+void mlir::vector::populateVectorSlicesTransformationPatterns(
+    OwningRewritePatternList &patterns, MLIRContext *context) {
----------------
aartbik wrote:
> 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!
Sorry I don't understand how the tuples get magically removed.
Both rewrites insert tuple/tuple_get but I do not see what patterns removes them concretely.
Maybe in these particular tests you wrote things degenerate into DCE (and that's great!), but I imagine it is possible to write tests where tuple ops will remain right?  




================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:800
+    OwningRewritePatternList &patterns, MLIRContext *context) {
+  patterns.insert<ExtractSlicesOpConversion, InsertSlicesOpConversion>(context);
+}
----------------
Also, let's plz rename to `XXXLowering` given the naming argument above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73295