[Mlir-commits] [mlir] [mlir][vector] Allow lowering multi-dim scatters to LLVM (PR #132227)

[Kunwar Grover]

Groverkss wrote:

> > Thanks for the addition! Could you add tests for the new pattern?
> > Basically, if we are reaching feature parity between `vector.gather` and `vector.scatter`, we should also make sure that the testing coverage is similar. I see two test files that exercise unrolling/lowering of `vector.gather`:
> > 
> > * https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/Vector/vector-gather-lowering.mlir
> > * https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/Vector/vector-transfer-unroll.mlir
> 
> I thought about this, and my reasoning about this is:
> 
> * This patch only adds a single pattern, UnrollScatter, which is used by ConvertToLLVM to lower multi-dimensional scatters. We have the same tests for gather for gather in test/Conversions/VectorToLLVM/vector-to-llvm.mlir . Previous patches added a negative test to lower multi-dimensional scatters, which this patch makes positive (i.e. we can lower them now).
> * test/Dialect/Vector/vector-gather-lowering.mlir tests are tests for unroll + linearize + lower to conditional loads for gather operations. Currently, the scatter op only implements unroll here, so adding tests here would not be correct.
> * test/Dialect/Vector/vector-transfer-unroll contains tests for a seperate style of unrolling, where a single dimension is unrolled into smaller chunks. This pattern unrolls the outer dimension completly. So these tests are unrelated.
> 
> Hopefully this makes it clear why the test coverage in this patch is enough.

Note that I do plan to add support for vector-gather-lowering and vector-transfer-unroll but I'm waiting till the work on passing index_vecs per dimension lands (https://discourse.llvm.org/t/rfc-improving-gather-codegen-for-vector-dialect/85011/12) because it makes the lowering not require linearization which makes it more robust. Just mentioning this in case you are wondering why this patch only adds one pattern.

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