[Mlir-commits] [mlir] [mlir][linalg] Reimplement SimplifyPackToExpandShape and SimplifyUnPackToCollapseShape for more cases. (PR #204971)

[Andrzej Warzyński]

banach-space wrote:

> > > Reimplement SimplifyPackToExpandShape and SimplifyUnPackToCollapseShape for more cases.
> > 
> > 
> > Could you summarise the newly supported cases? Thank you!
> 
> @banach-space Do I misunderstand your sentence? Should I rewrite the commit message `title` to show the cases? Maybe:
> 
> ```
> Reimplement SimplifyPackToExpandShape and SimplifyUnPackToCollapseShape for no transposition/padding semantic pack/unpack op.
> ```
> 
> original commit msg:
> 
> ```
> Reimplement SimplifyPackToExpandShape and SimplifyUnPackToCollapseShape for more cases.
> 
> If there is no transposition/padding semantic for pack/unpack,
> we could try to use `mlir::getReassociationIndicesForReshape()`
> to get the CollapseShapeOp/ExpandShapeOp form.
> ```

Yes, something along those lines. Basically, I am struggling to see what is currently supported and what new functionality is added. I've not looked at those transformations for a while and some context/reminder would help 😅 

In general, I find the tests in mlir/test/Dialect/Linalg/simplify-pack-unpack.mlir quite poorly structured - it's hard to tell negative from positive cases. Ideally, we should refactor that file following:
* https://mlir.llvm.org/getting_started/TestingGuide/#test-formatting-best-practices and
* https://mlir.llvm.org/getting_started/TestingGuide/#test-documentation-best-practices.

Specifically, any negative tests added here that would already fail without this change, should be submitted in a separate PR. Would you be open iterating a bit here? At the very least, we should clearly label the negative tests. If not, I can update the tests and then you can submit this as a follow-up. I would be more than happy to review your refactoring PR if you decided to that. Please let me know your preference.

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