Skip to content

[libc++] Reduce the compilation time required by SIMD tests #72602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 20, 2023

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Nov 17, 2023

Testing all the SIMD widths exhaustively is nice in theory, however in practice it leads to extremely slow tests. Given that

  1. our testing resources are finite and actually pretty costly
  2. we have thousands of other tests we also need to run
  3. the value of executing these SIMD tests for absolutely all supported SIMD widths is fairly small compared to cherry-picking a few relevant widths

I think it makes a lot of sense to reduce the exhaustiveness of these tests. I'm getting a ~4x speedup for the worst offender (reference_assignment.pass.cpp) after this patch.

I'd also like to make this a reminder to anyone seeing this PR that tests impact everyone's productivity. Slow unit tests contribute to making the CI slower as a whole, and that has a direct impact on everyone's ability to iterate quickly during PRs. Even though we have a pretty robust CI setup in place, we should remember that it doesn't come for free and should strive to keep our tests at a good bang for the buck ratio.

Testing all the SIMD widths exhaustively is nice in theory, however in
practice it leads to extremely slow tests. Given that
1. our testing resources are finite and actually pretty costly
2. we have thousands of other tests we also need to run
3. the value of executing these SIMD tests for absolutely all supported
   SIMD widths is fairly small compared to cherry-picking a few relevant
   widths

I think it makes a lot of sense to reduce the exhaustiveness of these
tests. I'm getting a ~4x speedup for the worst offender (reference_assignment.pass.cpp)
after this patch.

I'd also like to make this a reminder to anyone seeing this PR that
tests impact everyone's productivity. Slow unit tests contribute to
making the CI slower as a whole, and that has a direct impact on
everyone's ability to iterate quickly during PRs. Even though we have
a pretty robust CI setup in place, we should remember that it doesn't
come for free and should strive to keep our tests at a good bang for
the buck ratio.
@ldionne ldionne requested a review from a team as a code owner November 17, 2023 02:50
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 17, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 17, 2023

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Testing all the SIMD widths exhaustively is nice in theory, however in practice it leads to extremely slow tests. Given that

  1. our testing resources are finite and actually pretty costly
  2. we have thousands of other tests we also need to run
  3. the value of executing these SIMD tests for absolutely all supported SIMD widths is fairly small compared to cherry-picking a few relevant widths

I think it makes a lot of sense to reduce the exhaustiveness of these tests. I'm getting a ~4x speedup for the worst offender (reference_assignment.pass.cpp) after this patch.

I'd also like to make this a reminder to anyone seeing this PR that tests impact everyone's productivity. Slow unit tests contribute to making the CI slower as a whole, and that has a direct impact on everyone's ability to iterate quickly during PRs. Even though we have a pretty robust CI setup in place, we should remember that it doesn't come for free and should strive to keep our tests at a good bang for the buck ratio.


Full diff: https://github.com/llvm/llvm-project/pull/72602.diff

1 Files Affected:

  • (modified) libcxx/test/std/experimental/simd/test_utils.h (+2-2)
diff --git a/libcxx/test/std/experimental/simd/test_utils.h b/libcxx/test/std/experimental/simd/test_utils.h
index a478a43a87271cf..6954689d0876da7 100644
--- a/libcxx/test/std/experimental/simd/test_utils.h
+++ b/libcxx/test/std/experimental/simd/test_utils.h
@@ -28,7 +28,7 @@ struct TestAllSimdAbiFunctor {
 
   template <class T, std::size_t... Ns>
   void instantiate_with_n(std::index_sequence<Ns...>) {
-    (types::for_each(sized_abis<T, Ns + 1>{}, F<T, Ns + 1>{}), ...);
+    (types::for_each(sized_abis<T, Ns>{}, F<T, Ns>{}), ...);
   }
 
   template <class T>
@@ -36,7 +36,7 @@ struct TestAllSimdAbiFunctor {
     using abis = types::type_list<ex::simd_abi::scalar, ex::simd_abi::native<T>, ex::simd_abi::compatible<T>>;
     types::for_each(abis{}, F<T, 1>());
 
-    instantiate_with_n<T>(std::make_index_sequence<max_simd_size - 1>{});
+    instantiate_with_n<T>(std::index_sequence<1, 2, 8, 16, max_simd_size - 2, max_simd_size - 1, max_simd_size>{});
   }
 };
 

@ldionne
Copy link
Member Author

ldionne commented Nov 17, 2023

CC @EricWF @philnik777 @joy2myself

FWIW I get the following times after:

$ libcxx-lit build/default -sv libcxx/test/std/experimental/simd/ --param std=c++20 --time-tests

Slowest Tests:
--------------------------------------------------------------------------
11.61s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.reference/reference_assignment.pass.cpp
2.66s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_default.pass.cpp
2.27s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_broadcast.pass.cpp
1.49s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_subscr.pass.cpp
1.30s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_generator.pass.cpp
1.24s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.reference/reference_value_type.pass.cpp
1.17s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_ctor_default.pass.cpp
0.90s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_subscr.pass.cpp
0.84s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/memory_alignment.pass.cpp
0.83s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_ctor_broadcast.pass.cpp
0.76s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_width.pass.cpp
0.72s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_abi_tag.pass.cpp
0.69s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_alias.pass.cpp
0.69s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.reference/reference_alias.pass.cpp
0.68s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/simd_size.pass.cpp
0.63s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_simd.pass.cpp
0.60s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_simd_mask.pass.cpp
0.57s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_alias.pass.cpp
0.57s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_simd_flag_type.pass.cpp
0.54s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_width.pass.cpp

And before:

$ libcxx-lit build/default -sv libcxx/test/std/experimental/simd/ --param std=c++20 --time-tests
Slowest Tests:
--------------------------------------------------------------------------
45.70s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.reference/reference_assignment.pass.cpp
9.15s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_default.pass.cpp
7.09s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_broadcast.pass.cpp
4.51s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_subscr.pass.cpp
3.64s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_generator.pass.cpp
3.52s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.reference/reference_value_type.pass.cpp
3.02s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_ctor_default.pass.cpp
1.75s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_subscr.pass.cpp
1.28s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_ctor_broadcast.pass.cpp
1.14s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/memory_alignment.pass.cpp
1.08s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.reference/reference_alias.pass.cpp
1.04s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_alias.pass.cpp
0.96s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_width.pass.cpp
0.95s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_width.pass.cpp
0.88s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.mask.class/simd_mask_alias.pass.cpp
0.79s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_simd_flag_type.pass.cpp
0.76s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_simd.pass.cpp
0.73s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_abi_tag.pass.cpp
0.66s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/is_simd_mask.pass.cpp
0.62s: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.traits/simd_size.pass.cpp

reference_assignment.pass.cpp is still kinda beefy after the change so we should probably still look into improving that one specifically, but this is a first easy step.

@joy2myself
Copy link
Member

LGTM. I will try to improve reference_assignment.pass.cpp later and keep an eye on the compilation time of subsequent patches. Thanks for the reminder!

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with comments addressed and green CI. (And great branch name BTW)

@ldionne
Copy link
Member Author

ldionne commented Nov 17, 2023

No worries, thanks for working on SIMD! We love having a lot of test coverage, it's just that we sometimes lose track of how costly some tests can be :-)

Copy link

github-actions bot commented Nov 17, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

BTW. There is outdated (~1 year)
https://github.com/llvm/llvm-zorg/blob/ff329ad2df7a5f20d3d250ea52766036b6b748e8/zorg/buildbot/builders/sanitizers/buildbot_functions.sh#L356

I will try them again and let you know if they still slow.

@philnik777
Copy link
Contributor

No worries, thanks for working on SIMD! We love having a lot of test coverage, it's just that we sometimes lose track of how costly some tests can be :-)

Absolutely. I didn't realize how long the tests take until @vitalybuka pointed it out. It's really easy to miss the amount of instantiations with the generator tools we have now (they are way too nice to remove them over this little inconvenience though).

Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

This is a comment for the room, but there's a bigger problem with tests like this (than compile time).

That problem is complexity. Tests need to be correct upon inspection. There are no tests for our tests.

We need to be more discerning when writing tests.

@EricWF
Copy link
Member

EricWF commented Nov 18, 2023

(they are way too nice to remove them over this little inconvenience though).

We may have to disagree on that point. But like any tool, it's how they're used that matters.
Some tools encourage or make easy bad testing practices.

That said, testing types are one of the trickier things we have to do here.

@ldionne ldionne merged commit 5cd2475 into llvm:main Nov 20, 2023
@ldionne ldionne deleted the review/simd-tests-chill-out-a-bit branch November 20, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants