Page MenuHomePhabricator

[mlir] [integration-test] [VectorOps] Start an integration test directory for MLIR
ClosedPublic

Authored by aartbik on Wed, Jun 10, 6:30 PM.

Details

Summary

This CL introduces an integration test directory for MLIR in general, with
vector dialect integration tests in particular as a first working suite. To
run all the integration tests (and currently just the vector suite):

$ cmake --build . --target check-mlir-integration
[0/1] Running the MLIR integration tests
Testing Time: 0.24s
Passed: 22

The general call is to contribute to this integration test directory with more
tests and other suites, running end-to-end examples that may be too heavy for
the regular test directory, but should be tested occasionally to verify the
health of MLIR.

Background discussion at:
https://llvm.discourse.group/t/vectorops-rfc-add-suite-of-integration-tests-for-vector-dialect-operations/1213/

Diff Detail

Event Timeline

aartbik created this revision.Wed, Jun 10, 6:30 PM

Please have an extra careful look at the CMakeLists.txt and lit.cfg.py/lit.site.cfg.py.in files. I have no experience setting up a new testing directory inside the llvm repo, so most of this is a bit trail and error, and I welcome any constructive feedback on the setup.

aartbik edited the summary of this revision. (Show Details)Wed, Jun 10, 6:34 PM

Testing Time: 0.24s

As long as it is negligible, maybe we should enable the flag by default?

The separation unit-tests vs integration tests is nice so that we can still require bugfixes to come with a unit-tests.

LGTM overall (adding @stephenneuendorffer who spent time staring at CMake recently)

You likely need to disable these on windows (see mlir/test/mlir-cpu-runner/lit.local.cfg for an example) because of the lack of JIT support right now.

mlir/CMakeLists.txt
69

Actually I thought you'd introduce another CMakeOption MLIR_INCLUDE_INTEGRATION_TESTS to guard this (even if we enable it by default for now)

mlir/integration_test/CMakeLists.txt
4

I suspect this is a copy/paste from somewhere else?

aartbik updated this revision to Diff 270023.Wed, Jun 10, 7:39 PM
aartbik marked 3 inline comments as done.

addressed first round of comments, disabled windows run

You likely need to disable these on windows (see mlir/test/mlir-cpu-runner/lit.local.cfg for an example) because of the lack of JIT support right now.

Yes, sharp eye! All the window tests failed. Hopefull the lit.local.cfg in CPU avoids that now.

mlir/CMakeLists.txt
69

Actually, the target is defined as "check-integration-mlir" in the new CMakeList.txt I though this was just adding the rules if MLIR_INCLUDE_TESTS is set. That by itself is not a bad thing, right, or is it?

Or does this force the run? If so, what should this be?

mlir/integration_test/CMakeLists.txt
4

Yes, I had this as "look later if you need it" and forgot to remove.

mehdi_amini added inline comments.Wed, Jun 10, 8:11 PM
mlir/CMakeLists.txt
69

when you do add_lit_testsuites(check-integration-mlir without passing the option EXCLUDE_FROM_CHECK_ALL, then it is added by default as a dependency to check.

Here seems the most natural branching point to me (MLIR_INCLUDE_TESTS / MLIR_INCLUDE_INTEGRATION_TESTS)

LGTM. My main comment is that I don't know if I see the value of *LOTS* of integration tests. I'm used to using tests like this for design drivers, but it seems like you've tried to get some coverage of the vector dialect here?

mlir/integration_test/CMakeLists.txt
23

"check-mlir-integration" is more natural to me, since it exists under mlir/ Or do you expect that "check-integration" will be introduced as a dependence in other projects?

mlir/integration_test/Dialect/Vector/CPU/test-shuffle.mlir
7–25

Seems pretty simple for an integration test?

mlir/integration_test/CMakeLists.txt
25

I think "check-integration-mlir' here needs to be 'MLIR-INTEGRATION' matching the string in lit.cfg.py

mlir/integration_test/lit.cfg.py
19

This string should be referred to in add_lit_testsuites()

aartbik marked 8 inline comments as done.Thu, Jun 11, 12:00 PM
aartbik added inline comments.
mlir/integration_test/CMakeLists.txt
23

No, I just picked one. I changed it into what you suggested.

25

Done. Note that the MLIR_INTEGRATION_TEST_DEPS was inspired by the existing code as well, but looks undefined? Shouldn't this be MLIR_INTEGRATION_TEST_DEPENDS? I have changed it for now, please let me know if this is right or wrong?

mlir/integration_test/Dialect/Vector/CPU/test-shuffle.mlir
7–25

The initial suite is aimed at having one integration test per vector dialect operation that runs end-to-end from mlir file to llvm ir running in JIT. They are not necessarily exhaustive unit tests for the operation, and besides testing, also serve to supplement the documentation with real working examples. Of course, I expect the tests to grow organically. Also, by moving this to the public repo, our hope is to see lots of external contributions as well :-)

aartbik edited the summary of this revision. (Show Details)Thu, Jun 11, 12:01 PM
aartbik updated this revision to Diff 270207.Thu, Jun 11, 12:36 PM
aartbik marked 3 inline comments as done.

addressed second round of comments

aartbik updated this revision to Diff 270273.Thu, Jun 11, 6:08 PM

renamed substitution variable to reflect "integration" location better

Any more feedback? Can I proceed with the new framework?

Thanks for this!

This revision is now accepted and ready to land.Mon, Jun 15, 9:41 AM
nicolasvasilache accepted this revision.Mon, Jun 15, 9:48 AM

Thanks Aart!

This revision was automatically updated to reflect the committed changes.