[Mlir-commits] [mlir] [mlir][vector] Dispatch contraction lowering on iterator type (PR #231337)

[Andrzej Warzyński]

banach-space wrote:

Hey @FedericoBruzzone, thank you for working on this!

Dispatching based on iterator type rather than iterator position sounds like a solid improvement! In general, there's a lot of code around `vector.contract` that could be simplified and unified. In particular, I would like to align it with `linalg.contract` as much as possible (within reason).

That said, I feel that some of the complexity comes from handling "pure reduction" dimensions inside `vector.contract`. I'd prefer to represent those explicitly with `vector.multi_reduction` and keep `vector.contract` simpler. In fact, this is one of the changes I had in mind for [[#234238](https://github.com/llvm/llvm-project/issues/234238)](https://github.com/llvm/llvm-project/issues/234238) (sorry for not updating that earlier). To this end, I prepared:

* [[#237709](https://github.com/llvm/llvm-project/pull/237709)](https://github.com/llvm/llvm-project/pull/237709), together with a [[PSA on Discourse](https://discourse.llvm.org/t/mlir-vector-psa-disallow-pure-reduction-dims-in-vector-contract/92676)](https://discourse.llvm.org/t/mlir-vector-psa-disallow-pure-reduction-dims-in-vector-contract/92676).

I am also proposing [[#237708](https://github.com/llvm/llvm-project/pull/237708)](https://github.com/llvm/llvm-project/pull/237708).

Just to clarify, I do think that we should proceed with the approach you are proposing here. My suggestion is to land the changes above first, then build on them in this PR. That should leave less special-case handling for your refactoring to account for.

The timing is unfortunate - our PRs overlap, and I should have co-ordinated this better. Apologies for that!

Let me know what you think.

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