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

[Kunwar Grover]

Groverkss 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'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!

The current way of handeling this is correct and consistent. The LLVM type converter explicitly documents how vector types are converted to LLVM: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp#L629

I don't think we should bring the topic of what is the correct way here, as this PR is consistent with the rest of the lowerings.

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