[PATCH] D144108: [mlir][LinAlg][Transform] Add a transform op for conv2d to im2col

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added a comment.

In D144108#4129840 <https://reviews.llvm.org/D144108#4129840>, @mravishankar wrote:

> I know this pattern exists in IREE, but it is a bit of a hack. The representation of the im2col as a linalg.generic doesnt work as well. In reality it is similar to a gather. If this is upstreamed, it might be worth doing this right and not use an unnecessarily higher-dimensional operation for representing the im2col. Maybe we can chat offline?



In D144108#4131276 <https://reviews.llvm.org/D144108#4131276>, @ftynse wrote:

> These patterns look very similar, any chance they can be generalized somehow?

Please read my reply, this is why I wrote:

  As a followup, we should build a utility that is able to take 2 AffineDimExpr such that we have an indexing of the form:
  
  LHS(..., a * d0 + b * d1, ...), RHS(..., d1, ...), RES(..., d0, ...)
  and data-unroll/replicate the LHS along d1.
  
  This should be a small 1-D utility based on a simple inference that resembles what has been introduced in https://reviews.llvm.org/D142661 https://reviews.llvm.org/D143584.
  Then we can reuse the utility to work on any LinalgOp and avoid special casing 1-off C++ patterns.



In D144108#4131188 <https://reviews.llvm.org/D144108#4131188>, @qcolombet wrote:

> @nicolasvasilache, @mravishankar, I think it makes sense to upstream the version we have from IREE as is and iterate to make it better.
> If anything I believe it would make the review easier.
>
> In other words, how do you feel about having followup patches for changing the results to have several handles and using a gather like op?

It would make sense for me to start from an existing solution that we can then generalize.
If we want the generalization to exist first this would work too but I would then table this for a few days (maybe weeks) until we get it right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144108