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

Add SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS CMake option #771

Conversation

steffenlarsen
Copy link
Contributor

This commit adds the SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS as a shortcut for enabling all oneapi extension tests. It will only overwrite the other options if turned ON. In the OFF state the option has no effect.

This commit adds the SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS as a shortcut for
enabling all oneapi extension tests. It will only overwrite the other
options if turned ON. In the OFF state the option has no effect.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen steffenlarsen requested a review from a team as a code owner August 1, 2023 12:14
@bader
Copy link
Contributor

bader commented Aug 2, 2023

Can we make SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS to set a default value for the tests, which can be overridden by test specific macro?

Let's consider this set of options: SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS=ON SYCL_CTS_ENABLE_EXT_ONEAPI_MEMCPY2D_TESTS=OFF. With the current patch the second option is ignored.

@bader bader added the help wanted Extra attention is needed label Aug 3, 2023
@keryell
Copy link
Member

keryell commented Aug 10, 2023

Perhaps the semantics could be that SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS has an effect only on other options that have not explicitly set either ON or OFF.
But the problem with boolean options in CMake is that they are considered as OFF. https://cmake.org/cmake/help/latest/command/option.html
A solution could be to have these options as strings, so now we can have tri-state ON, OFF and nothing.
That said, is this something to pursue?

@psalz
Copy link
Contributor

psalz commented Aug 10, 2023

Perhaps the semantics could be that SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS has an effect only on other options that have not explicitly set either ON or OFF. But the problem with boolean options in CMake is that they are considered as OFF. https://cmake.org/cmake/help/latest/command/option.html A solution could be to have these options as strings, so now we can have tri-state ON, OFF and nothing. That said, is this something to pursue?

Couldn't this be achieved by simply passing SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS as the default value for each of the other options? I.e.,

add_cts_option(SYCL_CTS_ENABLE_EXT_ONEAPI_SUB_GROUP_MASK_TESTS
    "Enable extension oneAPI sub_group_mask tests" ${SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS})

@steffenlarsen
Copy link
Contributor Author

Couldn't this be achieved by simply passing SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS as the default value for each of the other options? I.e.,

add_cts_option(SYCL_CTS_ENABLE_EXT_ONEAPI_SUB_GROUP_MASK_TESTS
    "Enable extension oneAPI sub_group_mask tests" ${SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS})

That was my initial thought too, but the downside to it is that the other options won't change if you set SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS after the initial configuration.

The use-case in mind for this option was to allow for enabling all extension tests in the given group (here oneapi but the same strategy could be used wider) so automated systems won't have to be changed every time a new extension gets tests in the suite. As such a simple overwrite is sufficient, but if it is possible to make it work with selective disabling in tandem with this option, it shouldn't break the use-case.

@keryell
Copy link
Member

keryell commented Aug 10, 2023

@MathiasMagnus any feedback on this?

Copy link
Contributor

@MathiasMagnus MathiasMagnus left a comment

Choose a reason for hiding this comment

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

Functionally looks good, just one perf/stylistic change.

cmake/AddCTSOption.cmake Outdated Show resolved Hide resolved
@psalz
Copy link
Contributor

psalz commented Aug 14, 2023

That was my initial thought too, but the downside to it is that the other options won't change if you set SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS after the initial configuration.

Fair, but couldn't the same argument be made in reverse then? I.e., once SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS is set, it is no longer possible to selectively disable a subset of the extension tests?

The use-case in mind for this option was to allow for enabling all extension tests in the given group (here oneapi but the same strategy could be used wider) so automated systems won't have to be changed every time a new extension gets tests in the suite.

Yes that makes sense. In that scenario changing the configuration later on wouldn't be an issue though. The CI setup only has to make sure that SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS is set for the initial configuration. An alternative solution to this might be to ship a set of CMake presets.

@fraggamuffin
Copy link

needs another week

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

The solution sounds good for the envisioned use case.

Copy link
Contributor

@MathiasMagnus MathiasMagnus left a comment

Choose a reason for hiding this comment

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

Ship it!

@bader bader merged commit af335a7 into KhronosGroup:SYCL-2020 Sep 7, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants