[Mlir-commits] [mlir] [mlir][linalg] Extend elementwise (PR #124661)

[Andrzej Warzyński]

banach-space wrote:

> > * What's the benefit of lumping these ops under just one op? That is, is there a benefit to lumping arities?
> 
> On a single op, the enum business is complicated. Bit-field-like encoding helps clean up a bit, but it's still ugly. I'd rather see three ops (unary, binary, ternary), but again, that's my personal preference. If there's consensus on a single one, I'm also happy to acquiesce.

I've not seen this thread reach a conclusion, so here's my 2p. I haven't noticed any strong arguments for or against, but it sounds like the implementation would be simpler with separate ops. +1 to separating into three ops.

> ### Move `elemwise_unary` / `elemwise_binary`
> #### Pros
> * Clean cut. No new (temporary) ops initially, no old (deprecated) later
> * Simply move to ODS + affine maps, which is a practice we have agreed for other ops
> 
> #### Cons
> * More boilerplate in TableGen / C++, though the C++ part can be commoned up
> * Potential churn on current users, though we had little to none on `matmuls`
> 
> ### Create `element_wise`, move, deprecate `elemwise_unary` / `elemwise_binary`
> #### Pros
> * Single implementation reduces boilerplate in TableGen / C++
> * Less churn on current users, have time to move at their pace
> 
> #### Cons
> * Yet another operation that does the same thing as previous others
> * Enum business for unary/binary/ternary isn't trivial (future ABI issues?)
> * Users may _"never find the time to move"_ and we'll have duplicated ops for a long time

Hm, both options include `Move `elemwise_unary` / `elemwise_binary`? 😅 

My view on this is that, if possible, we should aim to reduce the disruptions to users. With `tensor.pack` + `tensor.unpack` it was a bit easier - I was moving Ops _verbatim_. Having `tensor.pack`/`tensor.unpack` + `linalg.pack`/`linalg.unpack` would be very confusing. In this case, it's not so clear-cut - we are updating the semantics.

Hm, both options involve Move elemwise_unary / elemwise_binary? 😅

My view is that, if possible, we should minimize disruptions for users. With tensor.pack + tensor.unpack, the transition was easier because I was moving ops verbatim. Having both tensor.pack/tensor.unpack and linalg.pack/linalg.unpack would have been too confusing. In this case, it's less clear-cut, as we're updating the semantics.

**My Suggestion:**
* If we converge toward a single op (`linalg.elementwise`), keep the old ops for a short transition period (e.g., <3 months). This gives users time to migrate smoothly.
* If we go with three separate ops (`linalg.elemwise_{unary|binary|ternary}`), then the only option is a direct replacement of the old definitions.

HTH, 
-Andrzej

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