[Mlir-commits] [mlir] [mlir][Vector] Support mixed mode vector.contract lowering (PR #117753)

[Kunwar Grover]

Groverkss wrote:

> Sorry if my comment reads critical. It was not the intent. This is a recurring issue, and I appreciate you bringing it up! Yet another half-baked thing we have in the Vector dialect and it’s probably a good time to address it.
> 
> The main challenge lies in embedding conversion semantics for each operand within `vector.contract`, as conversions can be complex (signedness, FMF, rounding/denormal modes and what not). Regardless of the current implementation, supporting only a few conversion cases would lead to an inconsistent representation and I think we shouldn’t move towards another inconsistent intermediate state _unless_ there is a clear path to a final state where full conversion semantics are supported. With this in mind, I see a couple of ways to move forward:
> 
> 1. Propose a way to encode conversion information per operand (e.g., using an attribute). This would involve translating explicit Arith/Vector conversion semantics into that embedded representation, which may or may not be worthwhile depending on what you are trying to achieve.
> 2. Take a step back, disallow conversions in `vector.contract` until we have a comprehensive solution and update the documentation accordingly.
> 
> What do you think?

Ok, thanks for making it clear with the full picture. This makes sense, it does seem half-baked.

I think that option 2 might be too big of change to plumb through, as many transformations need it. I know there are some mixed precision fadd intrinsics that use vector.contract as a way to target it, and might cause a lot of churn.

How about instead we do Option 1) in 2 parts:

- We update vector.contract documentation to mention explicitly how it handles extension for now. For integer types, it is already mentioned. We can also mention it for floating types (no extra flags for rounding / denormal modes / etc.). We can also mention that we would like to support this in future by allowing per operand extension information. This will make the lowering match what the documentation says and we can build on a restricted solution that matches the documentation for now. We can also mention that for now if the user has some other extension semantics, they should move the extension out of vector.contract themselves.
- I work on a patch on the side to implement FastMath/Denorm interfaces for vector.contract operand extension.

What do you think?


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