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

[Andrzej Warzyński]

banach-space wrote:

> Can you please say what 'test diffs' are here?

I meant IR diffs - that is, MLIR _before_ vs _after_ running a canonicalization or folding pass. Since this change doesn’t alter the final IR, the diffs should be empty (i.e., no changes to existing test output would be required).

>From what I understand, the new tests here illustrate a scenario where the change is beneficial (only **one invocation** of the folder is required), but the final IR is already correct, so the net IR diff is zero. (Please correct me if I'm mistaken.)

>  Is that running --canonicalize --debug and checking how many times the folder is called?

In my view, neither "canonicalize.mlir" nor "constant-fold.mlir" is appropriate for testing how many times the folder is invoked - and as far as I know, MLIR doesn’t currently test that sort of internal behavior.

If we do want to verify “single-pass folding” explicitly, we should introduce dedicated infra for that - for example, a purpose-built test pass like the one Diego proposed earlier.

That’s why I’m hesitant to use `-test-constant-fold` for this. It happens to run the folder once today, but that’s not its design goal, and that behaviour could change at any time.

> Do you mean duplicate them, once in canonicalize.mlir and once in constant-fold.mlir? 

No - I’d suggest having them only in canonicalize.mlir.

> Having it only in canonicalize.mlir would not test this PR, because --canonicalize will call the op's folder multiple times.

That’s true - but if the goal is to verify that the final IR is correct, then calling the folder multiple times isn’t a problem.

If the goal is instead to confirm that the fold happens on the first attempt, then I agree that requires more precise infra. However, we don’t test that kind of behavior elsewhere today, so I’d argue it’s not necessary to block this PR on that.

One possible compromise:
* Extract the tests into a separate PR (they should pass even without this change)
* In this PR, you could point to those tests and explain that the new logic ensures the same result is reached without requiring multiple fold iterations.

_To clarify - my main concern is with reusing `TestConstantFold` for something it wasn’t designed for._

If verifying that the folder runs only once is considered essential, then I’d suggest renaming `TestConstantFold` and adding some documentation to clarify its (updated) intended purpose. That way, future contributors will better understand what the pass is meant to test and why it behaves the way it does.

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