[Mlir-commits] [mlir] [mlir][math] Fix intrinsic conversions to LLVM for 0D-vector types (PR #141020)

[Andrzej Warzyński]

banach-space wrote:

> it should be possible to convert valid IR into valid IR

Agreed, in principle. My question is more about _how_ to enable this. Should we insert:
* `builtin.unrealized_conversion_cast %arg0 : vector<i32> to vector<1xi32>` (i.e., "view" rank-0 vectors as rank-1)?
* `builtin.unrealized_conversion_cast %arg0 : vector<i32> to i32` (i.e., treat rank-0 vectors as scalars)?
* Or insert `vector.extract f32 from vector<f32>` before hitting `ConvertMathToLLVM`? (That would require a dependency on `Vector`.)

My concern is that `builtin.unrealized_conversion_cast` can allow us to defer important design discussions. For example, I’d argue both `vector<i32>` and `vector<1xi32>` are sub-optimal if we're really just handling a scalar - in that case, shouldn't we lower it as a scalar?

That said, I realise you’re following the precedent set by the LLVM type converter:
https://github.com/llvm/llvm-project/blob/926c2013231a030a52037528bac0ba124c35ac32/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp#L630

So let me reframe the question: is converting `vector<f32>` --> `vector<1xf32>` the right choice in the context of the `Math` dialect?

> Otherwise, the point about conformant IR -> conformant IR lowering requirements comes up again.

Sure, though I think there's an implicit assumption here: that any form of Math or Vector should lower cleanly to LLVM. I don’t fully agree - and `rank-0` vectors are a good example. LLVM doesn’t support them directly, so we need some decomposition or transformation (just as we do for `vector.contrac`t despite having `ConvertVectorToLLVM`).

To be clear: I’m not blocking the current patch 🙂
If `builtin.unrealized_conversion_cast` works — and often it does, since it cancels out with other casts — that’s fine by me.

I don't want to come across as blocking this - definitely not :) If `builtin.unrealized_conversion_cast` works (it often does as it cancels with other instances of `builtin.unrealized_conversion_cast`) then so be it. 

> I'd really appreciate further guidance here.

If you don’t have strong opinions (or time to dig into all these questions 😅), I’d say proceed with what you have. My only asks would be:
* Leave TODOs for other unhandled ops (bonus points if you tackle them).
* Add a comment in `MathToLLVM.cpp` to explain the need for this conversion (e.g., "required to legalize rank-0 vectors").

Lastly, just to double-check - does `builtin.unrealized_conversion_cast work` in your E2E flow?

Thanks again!

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