[Mlir-commits] [mlir] [MLIR][Tensor] Canonicalize fully covering slice insertions into tensors with unit prefixes (PR #92912)

[llvmlistbot at llvm.org]

MaheshRavishankar wrote:

> > There might not be a single form. The two equivalent programs are equivalent and there is nothing to chose between them without additional context that only exist in the use case.
> 
> Sorry but I don't see any first principles coming from your statement here: in particular when it comes to a very simple case like here that only involve 1 op before and after the canonicalization, we ought to be able to select one!
> 
> Going back to this pattern: what makes it more difficult to match `tensor.expand_shape`  rather than `insert_slice`?
> We're in the same dialect, the same level of abstraction, I really don't see the argument against picking one side or the other here!
> 

This is exactly my point. There is no argument to pick one over the other. So what's stopping me from writing the reverse and add that to a canonicalization?

> >> If you have an expand shape in the input IR: would you do the opposite transformation?
> > No. IMO that is an opinionated decision. Should be left to the downstream users to pick what works for their context.
> 
> Unfortunately this isn't how MLIR Canonicalization is designed: you either canonicalize, or you don't.
> Actually we expose for canonicalize an option “disable-patterns” which takes a string list of patterns to filter out which kind of does what you want.

This option is supremely unergonomic and really cannot be used apart from simple use cases

> 
> Otherwise, feel free to propose a redesign of the system, but this is what we have!!
> 
> 

These kind of statements are unhelpful and do not lead to an outcome.


The above discussion is not helping the author land his change. I would suggest just adding a new `populate*Patterns` and adding this pattern there.

https://github.com/llvm/llvm-project/pull/92912