[mlir][vector] What passes should we test in vector-to-llvm.mlir?

[banach-space]

As highlighted in this comment, tests in:

• test/Conversion/VectorToLLVM/vector-to-llvm.mlir

currently exercise the full ConvertVectorToLLVMPass. This is due to the use of -convert-vector-to-llvm in that file. However, given the location of this test file, it should focus exclusively on testing "Vector to LLVM conversion patterns," specifically:

• populateVectorToLLVMConversionPatterns.

These conversion patterns are included in ConvertVectorToLLVMPass, which is why we are using -convert-vector-to-llvm. However, it is important to ensure that "vector-to-llvm.mlir" tests only the conversions from populateVectorToLLVMConversionPatterns. Tests for other patterns should be moved to separate, appropriate test files.

Proposed Actions:

• Audit "vector-to-llvm.mlir" to ensure that it only tests conversions from populateVectorToLLVMConversionPatterns.
• Relocate tests for other patterns to "test/Dialect/Vector" or other suitable test files.

Thanks for taking a look!
-Andrzej

CC @dcaballe @Groverkss @hanhanW

[llvmbot]

@llvm/issue-subscribers-mlir

Author: Andrzej Warzyński (banach-space)

As highlighted in [this comment](https://github.com//pull/121440#issuecomment-2608058594), tests in:

• test/Conversion/VectorToLLVM/vector-to-llvm.mlir

currently exercise the full ConvertVectorToLLVMPass. This is due to the use of -convert-vector-to-llvm in that file. However, given the location of this test file, it should focus exclusively on testing "Vector to LLVM conversion patterns," specifically:

• populateVectorToLLVMConversionPatterns.

These conversion patterns are included in ConvertVectorToLLVMPass, which is why we are using -convert-vector-to-llvm. However, it is important to ensure that "vector-to-llvm.mlir" tests only the conversions from populateVectorToLLVMConversionPatterns. Tests for other patterns should be moved to separate, appropriate test files.

Proposed Actions:

• Audit "vector-to-llvm.mlir" to ensure that it only tests conversions from populateVectorToLLVMConversionPatterns.
• Relocate tests for other patterns to "test/Dialect/Vector" or other suitable test files.

Thanks for taking a look!
-Andrzej

[dcaballe]

Not sure if my understanding of the problem is correct. We know the lowering from Vector to LLVM is intricate, involving quite a few patterns and intermediate stages. ConvertVectorToLLVMPass applies all of those patterns and we have to make sure that is working properly and, therefore, it needs to be tested as a whole. If we want to test the different populate* patterns independently, we may want to create a new test pass(es) to apply only those patterns in isolation and add more tests? We shouldn't remove tests from test/Conversion/VectorToLLVM/vector-to-llvm.mlir if there are somewhat testing a Vector-to-LLVM end-to-end use case, right? What am I missing?

[banach-space]

We know the lowering from Vector to LLVM is intricate, involving quite a few patterns and intermediate stages. ConvertVectorToLLVMPass applies all of those patterns and we have to make sure that is working properly and, therefore, it needs to be tested as a whole.

Agreed. That said, I’m concerned we might be conflating "integration/lowering" tests and "conversion" tests (*).

If we want to test the different populate* patterns independently, we may want to create a new test pass(es) to apply only those patterns in isolation and add more tests?

In fact, a new "pass" is "effectively" added in 121440 (**). My main concern, however, is avoiding duplication. Specifically:

• We shouldn’t introduce yet another test file with yet another redundant set of tests.

This is how duplication tends to creep in. Instead, tests in test/Conversion/VectorToLLVM/vector-to-llvm.mlir that only rely on:

• populateVectorToLLVMConversionPatterns

(i.e., tests that lower to LLVM without additional patterns from ConvertVectorToLLVMPass::runOnOperation) should be extracted into a dedicated file.

We shouldn't remove tests from test/Conversion/VectorToLLVM/vector-to-llvm.mlir if there are somewhat testing a Vector-to-LLVM end-to-end use case, right?

Exactly. This isn’t about removing tests but about separating them into:

1. Basic conversion tests for populateVectorToLLVMConversionPatterns.
2. Integration/lowering tests for ConvertVectorToLLVMPass::runOnOperation.

This reorganization will enable finer-granularity testing while avoiding redundancy. It’s not about adding more tests unless there are genuine gaps, which I don’t think is the case.

Thanks for your input!

(*) That’s how I interpret ConvertVectorToLLVMPass::runOnOperation, though admittedly it’s borderline.
(**) Strictly speaking, it's adding a missing interface for Vector dialect.

[dcaballe]

I think we are almost aligned on this but I wouldn't "extract" tests from the current conversion pass. Even if those tests are lowered by a single populate..., we have to make sure that no other current and future pattern added to that pass messes up the lowering. So definitely +1 to adding unit tests for individual populate... patterns but I would keep the existing integration tests as are. Does it make more sense now?

[banach-space]

IIUC, you want to make sure we preserve the same level of test coverage for -convert-vector-to-llvm. Which makes sense!

Even if those tests are lowered by a single populate..., we have to make sure that no other current and future pattern added to that pass messes up the lowering.

Sure, but this is actually achievable with what I am proposing :) If we extract the relevant tests to a dedicated test file, then we can use two RUN lines that should be compatible and that will verify both flows:

// Only test populateConvertToLLVMConversionPatterns
// RUN: mlir-opt %s -convert-vector-to-llvm -split-input-file | FileCheck %s

// Test full Vector-to-LLVM pipeline
// RUN: mlir-opt --convert-to-llvm="filter-dialects=vector" --split-input-file %s | FileCheck %s

// ... existing and new tests

This way the test coverage is preserved and we avoid duplicating test cases. If the generated output is not identical, we can use FileCheck prefixes to control that. This way we also get to see what the difference between two "pipelines" is. WDYT?

My main concern is a proliferation and duplication of tests that don't improve our test coverage but instead create maintenance burden.

[dcaballe]

Oh, yes, that makes sense to me :)

[banach-space]

Here's a patch:

• [mlir][vector] Categorize Vector-to-LLVM conversion tests (nfc) #125918

Vast majority of tests either work identically with both lowering paths, i.e. via:

• -convert-vector-to-llvm, and
• -convert-to-llvm="filter-dialects=vector" (introduced in [mlir][vector] Create VectorToLLVMDialectInterface #121440).

The more complex ones, do require:

• -convert-vector-to-llvm,

but that's expected.

Sadly, after re-basing on top of main, there's some new issues (including the LLVM CI complaining about undefs - funny timing!). I will report back once I have time to update them.