This is an archive of the discontinued LLVM Phabricator instance.

[mlir][test] Refactor SparseTensor CPU integration tests
ClosedPublic

Authored by awarzynski on Jul 30 2023, 12:40 PM.

Details

Summary

SUMMARY OF CHANGES

This patch aims to reduce test duplication and to improve code re-use in
SparseTensor integration tests for CPU. This is a direct follow-up of:

  1. https://reviews.llvm.org/D155403 (test duplication), and
  2. https://reviews.llvm.org/D155405 (code re-use),

The key logic for this patch is implemented in:

  • SparseTensor/CPU/lit.local.cfg.

Essentially, the set-up that used to be repeated across all test files
has been extracted into a common LIT configuration file. This makes code
re-use straightforward.

All SVE/VLA tests are now enabled _conditionally_ and refactored to use
mlir-cpu-runner rather than lli. The former helps with test
duplication and the latter with code re-use.

A few additional refactoring changes are included.

  1. The reduce verbosity, long runtime library names like:
%mlir_native_utils_lib_dir/libmlir_c_runner_utils%shlibext

are replaced with:

%mlir_c_runner_utils
  1. In order to keep the code and the comments in sync, and to maintain consistency across the tests, the following:
enable-runtime-library=true

is swapped with (and vice-versa):

enable-runtime-library=false

Note that this change won't affect test coverage. Only few tests
required such update.

  1. A VLS vectorization RUN line is added in tests where there was a VLA/VLS RUN line, but no VLS RUN line (with a few exceptions of tests that only contained one RUN line to begin with).
  1. A few test variables are renamed/added. Most notable example:
    • %{options} --> %{sparse_compiler_opts}

TEST RUNTIME IMPROVEMENT

Tl;Dr This change improves test execution time by ~25%.

At the moment, the following llvm-lit invocation takes ~7.30s on my
AArch64 workstation (with SVE):

llvm-lit  <llvm-project>/mlir/test/Integration/Dialect/SparseTensor/CPU/

This timing doesn't change no matter what the value of the following
CMake variable is (it should enable/disable SVE/VLA tests):

MLIR_RUN_ARM_SVE_TESTS

With this patch, the execution time will indeed depend on the value of
the above CMake variable:

  • with MLIR_RUN_ARM_SVE_TESTS=true the timing remains intact,
  • with MLIR_RUN_ARM_SVE_TESTS=false the timing drops to ~5.40s (~25% improvement).

This is expected:

  • on average there are 4 RUN lines per test,
  • _without this change_ (and with MLIR_RUN_ARM_SVE_TESTS=false) the 4th RUN line would in most cases duplicate the 3rd RUN line,
  • _with this change) (and with MLIR_RUN_ARM_SVE_TESTS=false) the 4th RUN line becomes empty.

PATCH SIZE

While rather large and touching many files, most changes in this patch
are rather mechanical. All test configurations have been preserved and
only in a handful of cases new RUN lines added.

Diff Detail

Event Timeline

awarzynski created this revision.Jul 30 2023, 12:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 12:40 PM
awarzynski requested review of this revision.Jul 30 2023, 12:40 PM
awarzynski edited the summary of this revision. (Show Details)Jul 30 2023, 12:46 PM
c-rhodes accepted this revision.Jul 31 2023, 3:35 AM

Thanks Andrzej this is a nice refactor, I haven't reviewed every single test as there's a fair amount of churn but generally LGTM.

This revision is now accepted and ready to land.Jul 31 2023, 3:35 AM
Matt added a subscriber: Matt.Jul 31 2023, 12:11 PM
aartbik accepted this revision.Aug 2 2023, 12:57 PM

Thanks for following up on this!

mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_0.mlir
24

so same here, not repeated anymore

Thanks for reverting and apologies for the disruption - that failing test is disabled for AArch64 and I forgot to test on X86 :(

I am also told that this is causing some pain downstream, so please expect a bit of a delay.

@aartbik, this is unrelated to my change, but on X86 I hit this with ToT llvm_unreachable.

awarzynski reopened this revision.Aug 3 2023, 2:15 AM
This revision is now accepted and ready to land.Aug 3 2023, 2:15 AM
awarzynski updated this revision to Diff 546767.Aug 3 2023, 2:26 AM

Remove undefined and unused LIT variable

This will fix the bot failure, but not the downstream issue.

aartbik requested changes to this revision.Aug 7 2023, 9:00 AM

This will still get the DEFINE always change, right?

This revision now requires changes to proceed.Aug 7 2023, 9:00 AM

Refactor all tests not to rely on lit.local.cfg

Instead of relying on shared logic in lit.local.cfg, the generic test logic is
duplicated across all tests files. This is required as there are downstream
users that do not use lit.local.cfg.

CC @aartbik

Add missing set-up in sparse_tanh.mlir

aartbik accepted this revision.Aug 10 2023, 4:33 PM

I double checked this will keep working for "certain downstream users" ;-)
And thanks for your patience doing this refactoring, much appreciated

(and looking forward to the day we can move the shared part out some place!)

mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_0.mlir
5

"some downstream users" very diplomatic ;-)
And thanks for that!

This revision is now accepted and ready to land.Aug 10 2023, 4:33 PM

Re-landed :) 🎉

mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_0.mlir
5

I think that a lot of people (like my "future self") might feel the urge to move this to lit.local.cfg. So I wanted to leave a hint not to :) I've polished this sentence a tiny bit before landing.

mehdi_amini added inline comments.Sep 12 2023, 1:06 PM
mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate_dim_0.mlir
5

I have concerns about the precedent here: we should not support "non-compatible lit environment", this does not seem like a reasonable requirement from every downstream to push this on upstream!