[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:800
+
+void mlir::vector::populateVectorSlicesTransformationPatterns(
+    OwningRewritePatternList &patterns, MLIRContext *context) {
----------------
nicolasvasilache wrote:
> 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?  
> 
> 
Well if of course does not guarantee that all tuple ops are eliminated (it only deals with slices ops rewriting), since tuple values may "leak" going in already. Take for example

func @extract_slices(%arg0: vector<3x3xf32>) -> tuple<vector<2x2xf32>, vector<2x1xf32>, vector<1x2xf32>, vector<1x1xf32>> {
  %0 = vector.extract_slices %arg0, [2, 2], [1, 1]
    : vector<3x3xf32> into tuple<vector<2x2xf32>, vector<2x1xf32>, vector<1x2xf32>, vector<1x1xf32>>
  return %0 : tuple<vector<2x2xf32>, vector<2x1xf32>, vector<1x2xf32>, vector<1x1xf32>>
}

this will be lowered to 

 func @extract_slices(%arg0: vector<3x3xf32>) -> tuple<vector<2x2xf32>, vector<2x1xf32>, vector<1x2xf32>, vector<1x1xf32>> {
    %0 = vector.strided_slice %arg0 {offsets = [0, 0], sizes = [2, 2], strides = [1, 1]} : vector<3x3xf32> to vector<2x2xf32>
    %1 = vector.strided_slice %arg0 {offsets = [0, 2], sizes = [2, 1], strides = [1, 1]} : vector<3x3xf32> to vector<2x1xf32>
    %2 = vector.strided_slice %arg0 {offsets = [2, 0], sizes = [1, 2], strides = [1, 1]} : vector<3x3xf32> to vector<1x2xf32>
    %3 = vector.strided_slice %arg0 {offsets = [2, 2], sizes = [1, 1], strides = [1, 1]} : vector<3x3xf32> to vector<1x1xf32>
    %4 = vector.tuple %0, %1, %2, %3 : vector<2x2xf32>, vector<2x1xf32>, vector<1x2xf32>, vector<1x1xf32>
    return %4 : tuple<vector<2x2xf32>, vector<2x1xf32>, vector<1x2xf32>, vector<1x1xf32>>
  }

Here the tuple leaks, because there is no way to remove it. However, the lowering guarantees that any uses of slices where the tuple values are consumed, are lowered into something without tuples, even for the newly introduced operations.

I have added the comment on the lowering API a bit to reflect this.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73295