Skip to content
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

Make math_builtin_api tests splittable into smaller chunks #889

Closed
wants to merge 1 commit into from

Conversation

jzc
Copy link
Contributor

@jzc jzc commented Apr 30, 2024

Currently, the some of the generated math_builtin_api tests are huge. These are the current line counts:

   72405 tests/math_builtin_api/math_builtin_integer.cpp
   27088 tests/math_builtin_api/math_builtin_float_half.cpp
   27088 tests/math_builtin_api/math_builtin_float_double.cpp
   27079 tests/math_builtin_api/math_builtin_float_base.cpp
   17218 tests/math_builtin_api/math_builtin_relational_base.cpp
    4599 tests/math_builtin_api/math_builtin_relational_half.cpp
    4599 tests/math_builtin_api/math_builtin_relational_double.cpp
    4307 tests/math_builtin_api/math_builtin_common_half.cpp
    4307 tests/math_builtin_api/math_builtin_common_double.cpp
    4298 tests/math_builtin_api/math_builtin_common_base.cpp
    3689 tests/math_builtin_api/math_builtin_native.cpp
    3689 tests/math_builtin_api/math_builtin_half.cpp
    1163 tests/math_builtin_api/math_builtin_geometric_base.cpp
     832 tests/math_builtin_api/math_builtin_geometric_double.cpp

When building on release mode, -O3 optimization will be used, which aside from making compilation taking much longer, will make building math_builtin_integer.cpp use an unreasonable amount of RAM for the average person (64GB+).

This PR adds a functionality to split these tests into smaller chunks, which allows for each chunk to be compiled with a reasonable amount of RAM, and leads to a much faster compilation time.

Each of the original generated files listed above are all split into a test-wide configurable amount SYCL_CTS_MATH_BUILTIN_N_SPLITS, which is 16 initially. This makes each of the chunks of the biggest test math_builtin_integer.cpp under 5k LOC.

@jzc jzc requested a review from a team as a code owner April 30, 2024 23:19
continue()
endif()
foreach(split_index RANGE 1 ${SYCL_CTS_MATH_BUILTIN_N_SPLITS})
if ("${cat}" STREQUAL geometric AND "${var}" STREQUAL half)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that this if means that geometric_half is small enough by itself to be split, but a comment would be helpful here about this exception

math(EXPR split_index_0 "${split_index} - 1")
generate_cts_test(TESTS TEST_CASES_LIST
GENERATOR "generate_math_builtin.py"
OUTPUT "math_builtin_${cat}_${var}_${split_index}_${SYCL_CTS_MATH_BUILTIN_N_SPLITS}.cpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need SYCL_CTS_MATH_BUILTIN_N_SPLITS in the name?
If we decide to preserve it, would it be better to add _of_ between split index and total number of splits to make the name more human-readable?

@keryell
Copy link
Member

keryell commented Sep 10, 2024

@jzc @steffenlarsen How does this compare to #927?

@steffenlarsen
Copy link
Contributor

@jzc @steffenlarsen How does this compare to #927?

I did not even realize @jzc had a patch like this too. They look very similar, so I feel bad for intercepting it with my patch. Sorry, @jzc !

@keryell
Copy link
Member

keryell commented Sep 25, 2024

@jzc Are there more improvements to do on top of #927?

@jzc
Copy link
Contributor Author

jzc commented Oct 21, 2024

@keryell No, Steffen's patch should cover it, closing this PR.

@jzc jzc closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants