[Mlir-commits] [mlir] [mlir][linalg] Introduce transpose semantic to 'linalg.matmul' ops. (PR #104783)

[Renato Golin]

rengolin wrote:

> Yeah, but this seems to transient in terms of dialect/op design. We have a permutation map that is not an indexing map. It changes all the assembly formatting, and then we have to change that again.

I agree the permutation map isn't the best way to represent this. But the indexing map is also a "change in syntax". So, perhaps we should just pause and talk about the _final_ `matmul` syntax (independent of `contract`) and stick with it.

> I really dont see the point of adding this new operation. Its just combining transposes, but then what about fusing with broadcasts. We have discussed this previously, that pretty much any forward looking change has to account for broadcasting behavior of operands to matmuls.

This is not a _new_ operation. It's just `matmul`. Users of any existing operations in linalg should see absolutely no difference in their IR.

We're not combining transposes, we're starting the discussion on how to represent both transpose and broadcast. Perhaps the attribute has thrown it off the course, so @shahidact I recommend we stick to affine maps from now on and get both transpose and broadcast in one PR.

However, I'd prefer to have this discussion over `matmul` _only_ for now, and then just apply the same ideas to `match_matmul` and `batch_reduce_matmul` later. This means we have time to adapt and find issues before they become intrinsic to the design.

> All I am suggesting is that instead of adding a `linalg.matmul` that has implicit permutation maps create a separate operation (which wont have name collision)

We're not adding a new op, there is no name collision. All users of `matmul` will continue using `matmul` like they've always done. There is no change in behaviour or syntax to any current user of `matmul`.

> * Change the op name to `linalg.contraction` to avoid unnecessary collision with opdsl ops. Let these be two separate worlds.

That's a non-starter. Contraction is way more complicated than matmul and not viable in the short term. We'll be stuck in bike-shedding for months before any of this becomes useful and we'll end up with multiple variations of syntax.

We **will** do that too very soon! But these must be orthogonal changes for us to make forward progress on the existing models.

> * Instead of permutation vectors use indexing maps

We'll update this PR with affine maps for both transpose and broadcast (that was work in progress), but on the existing `matmul`. This will **NOT** change the semantics or syntax of the current `matmul` operation at all _if you don't use the new maps_.

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