[Mlir-commits] [mlir] [mlir][vector] Add more tests for ConvertVectorToLLVM (2/n) (PR #102203)

[Andrzej Warzyński]

banach-space wrote:

> Does it make sense to support this usecase in the future or do we consider this should never be generated and the user never write it ?

It's always good to have a motivating example - can you think of a situation where that could be used? Nothing on my mind today, but I tend to look at a rather limited set of examples.

In general, I always advocate for good balance between complexity and what's actually needed. In particular with scalable vectors, we tend to discover that we don't really need every single pattern in the Vector dialect to support them. And we still manage to generate good code 😅  (just as an example)

Having said that, I'm not opposed to improving the support for scalable vectors.

> I have seen another MR recently (from you I think) which did not handle the lowering to a loop.
> 
> ```
>       // This restriction reflects what's currently supported in terms of
>       // scalable vectors. However, we could relax this if there's a use case.
> ```
> 
> In case I get into sorting this out, do you think we should have some separate scalable vectors canonicalization patterns a bit like ArmSMEToSCF but without the SME dependency or simply have lowerVectorToLLVM support this case is enough ?

Depends on complexity. For more involved logic, I'd start with something "hidden" in the SVE dialect. And then, if we find that particularly useful, we could "elevate" that to Vector. Having said that, this particular functionality would be valid for any implementation of scalable vectors, not only SVE.

Btw, I really appreciate all the reviews and wanted to say that I'm always happy to return the favour. In particular, I'm reviewing all Vector tests and if you ever feel like taking a look at one of the tests yourself, here's a list that I use to co-ordinate this:
* https://github.com/orgs/llvm/projects/23

Sharing mostly so that you are aware :)

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