[PATCH] D80772: [mlir] [VectorOps] Use 'vector.flat_transpose' for 2-D 'vector.tranpose'

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added inline comments.
This revision is now accepted and ready to land.
Herald added a project: MLIR.


================
Comment at: mlir/test/Dialect/Vector/vector-flat-transforms.mlir:5
+//
+// TODO(ajcbik,ntv): having ShapeCastOp2DDownCastRewritePattern and
+//                   ShapeCastOp2DUpCastRewritePattern too early in
----------------
aartbik wrote:
> nicolasvasilache wrote:
> > aartbik wrote:
> > > sending this out for some early discussion; this hits a phase ordering issue
> > > 
> > > I propose to move the shapecast lowering a bit later, so we can fold them first and only lower them when they cannot be eliminated
> > I think this should be resolved in a separate revision and this is fine for now.
> > 
> > Note that this is more an order of visitation problem. Given:
> > ```
> > %a = shape_cast %0
> > %b = shape_cast %a
> > ```
> > where the shape casts can fold, if %a is visited before %b then it will be expanded.
> > 
> > I have seen this type of behavior a bunch of times in different place (albeit not involving folding + canonicalization + lowering IIRC).
> > 
> > Seems like ShapeCast should have a canonicalizer / canonicalization pattern (`hasCanonicalizer=1`) with a separate match and rewrite.
> > Then ShapeCastLowering could query that on all its uses and fail to lower if any foldable use is left.
> > 
> > In other words, this type of pattern ordering can be resolved by finer-grained case disjunction.
> > However this seems like it can still "miss folding at a distance": consider a chain of transposes that are lowered in some arbitrary order introducing reshapes. 
> > Folding opportunities would only appear if consecutive transposes are lowered before any newly introduced shape_cast is visited.
> > This seems like the worklist-based algorithm would handle this ordering naturally but I imagine we can construct more intricate cases where this would not be true? 
> > 
> > Pinging @rriddle to see if there are more idiomatic ways of doing this, if this should be integrated in the rewriter itself (i.e. delay pattern application if any operand has folding opportunities), or something else.
> yes, sounds good fixing this later somehow; the TODO reminds us to follow up :-)
Canonicalizer doesn't run in any pattern rewriting (it can in the Nicolas's multi-level driver if you configure it that way), folding is run in both greedy rewriter and dialect converter. However, folding does not recurse upwards on operands of the given op, which it seems what you need here. In dialect conversion, there's a TODO comment related to potentially folding any operation it visits even if it is considered legal. Maybe that will help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80772