[Mlir-commits] [mlir] [mlir][vector] Support complete folding in single pass for vector.insert/vector.extract (PR #142124)

[Andrzej Warzyński]

banach-space wrote:

Thanks!

> The existing `Dialect/Vector/canonicalize.mlir` file doesn't seem to be a good place to add test for this change, as canonicalization performs repeated folding, making it impossible to verify whether the optimization occurs in a single pass.

To me, this raises the broader question:
* "How do we test these patterns?". 

If the goal is to verify that the fold happens in a single pass of the folder, that’s effectively testing an **implementation detail** or **driver behaviour** - and as far as I know, we don’t typically test that. Instead, our tests tend to focus on simple input vs. output IR comparison.

> I happened to see an existing pass that implements what you said. Do you think it is a good choice to use [TestConstantFold](https://github.com/llvm/llvm-project/blob/main/mlir/test/lib/Transforms/TestConstantFold.cpp#L16) for folding tests?

I’m not particularly in favor of using that. It works in this case, but it was designed for a slightly different purpose. Using it here would just introduce inconsistency in how we test folding, and nothing prevents someone from later refactoring it to **run the folder multiple times**, which would break the test's assumptions.

In my view, we’re simply missing folding tests for `vector.insert` / `vector.extract` in "canonicalize.mlir". This becomes clear when you look at the two cases here:
https://github.com/llvm/llvm-project/blob/b4b86a7a3c2b2ad6cdb6c1e1041ce28ee4a63a17/mlir/test/Dialect/Vector/canonicalize.mlir#L3265-L3289

Notably, there’s no test that shows `vector.insert` / `vector.extract` being folded away entirely - like the examples you're adding here  (unless I missed sth). Once such tests exist, a change like yours would ideally be validated by the absence of test diffs, which is more robust.

This would also be easier to catch if:
* we clustered all folding tests for `vector.insert` and `vector.extract` together, and/or
* moved them to a dedicated file.

At a minimum, I recommend moving the new tests to "canonicalize.mlir", so we at least maintain consistency in where we test folding behaviour.

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