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

[Andrzej Warzyński]

banach-space wrote:

> 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.

Thanks for the explanation and the thoughtful analysis - that makes sense!

That said, I think it would still be valuable to include some finer-grained testing, for a couple of reasons:

* While LLVM is probably the most widely used and well-tested target for Vector (*), it's not the only one — so it's important that we have coverage across other parts of the stack.
* `ConvertVectorToLLVM` is a large _integration_ pipeline — it exercises many transformations together. It's helpful to complement that with more focused _unit tests_ to ensure the individual pieces are working in isolation.

Looking at your patch, it seems like this TD op is a good fit for testing the new `UnrollScatter` pattern directly:
* https://mlir.llvm.org/docs/Dialects/Transform/#transformapply_patternsvectorlower_gather-transformapplylowergatherpatternsop

Now, even though this patch is only about **scatter**, I think it would be great and valuable to add symmetric tests for both **gather** and **scatter** - probably in a new dedicated test file. I realise this goes beyond the immediate scope though, so this is just a kind request.

Also, this could be a good opportunity to rename the op to something like:

* `transform.apply_patterns.vector.lower_gather_scatter`

Thanks again for all the work here — it’s coming together nicely!

-Andrzej

(*) Just an assumption based on experience.

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