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

Split group_scan and group_reduce tests by data types #815

Merged
merged 12 commits into from
Nov 21, 2023

Conversation

jiezzhang
Copy link
Contributor

To reduce overhead on template instantiation, this commit split group_reduce and group_scan tests by data type.

Besides, this commit remove 'fp16' and 'fp64' from 'get_std_type' if corresponding cmake options are set.

@jiezzhang jiezzhang requested a review from a team as a code owner October 16, 2023 06:49
@bader bader added the help wanted Extra attention is needed label Oct 16, 2023
tests/group_functions/group_reduce.h Outdated Show resolved Hide resolved
@bader bader requested a review from keryell November 8, 2023 16:29
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.

Almost there!

tests/group_functions/CMakeLists.txt Outdated Show resolved Hide resolved
@bader bader requested a review from keryell November 10, 2023 18:57
@bader
Copy link
Contributor

bader commented Nov 14, 2023

Besides, this commit remove 'fp16' and 'fp64' from 'get_std_type' if corresponding cmake options are set.

I think, this deserves a separate PR. Multiple tests use get_std_type function (e.g. vector_* tests) and this change reduces the test scope for non-group tests.
If we need to remove testing half and doubles, I suggest we apply half_double_filter to group_functions tests only. But it's not clear why we would want to remove tests for these data types. Could you clarify?

@maarquitos14
Copy link
Contributor

I'm trying out this branch locally and I'm seeing that the files that are now generated from cpp.in files are not really built. I'm investigating it, but I think this should not be merged until this is clear.

@jiezzhang have you verified that all the tests are built and ran? Maybe I'm misconfiguring anything?

Co-authored-by: Marcos Maronas <[email protected]>
@jiezzhang
Copy link
Contributor Author

I'm trying out this branch locally and I'm seeing that the files that are now generated from cpp.in files are not really built. I'm investigating it, but I think this should not be merged until this is clear.

@jiezzhang have you verified that all the tests are built and ran? Maybe I'm misconfiguring anything?

Thank you! Fixed

@jiezzhang
Copy link
Contributor Author

If we need to remove testing half and doubles, I suggest we apply half_double_filter to group_functions tests only. But it's not clear why we would want to remove tests for these data types. Could you clarify?
"-DSYCL_CTS_ENABLE_DOUBLE_TESTS" and "-DSYCL_CTS_ENABLE_HALF_TESTS" will enable/disable fp64 and fp16 tests. I think generated code should follow this behavior.
Since FP64 and FP16 is optional feature, some implementation may not compile these source files correctly (like AOT compilation flow).

@bader
Copy link
Contributor

bader commented Nov 16, 2023

If we need to remove testing half and doubles, I suggest we apply half_double_filter to group_functions tests only. But it's not clear why we would want to remove tests for these data types. Could you clarify?
"-DSYCL_CTS_ENABLE_DOUBLE_TESTS" and "-DSYCL_CTS_ENABLE_HALF_TESTS" will enable/disable fp64 and fp16 tests. I think generated code should follow this behavior.
Since FP64 and FP16 is optional feature, some implementation may not compile these source files correctly (like AOT compilation flow).

Thanks for clarification. I would refactor the code to make it clearer. We should not add optional types unconditionally to the list of standard types in the first place. They should be added only after option value check. Adding with follow-up removing is confusing.
Let's do this refactoring in a separate PR.

@jiezzhang
Copy link
Contributor Author

jiezzhang commented Nov 17, 2023

Thanks for clarification. I would refactor the code to make it clearer. We should not add optional types unconditionally to the list of standard types in the first place. They should be added only after option value check. Adding with follow-up removing is confusing.
Let's do this refactoring in a separate PR.

Sure. I have moved half_double_filter into group_functions/CMakeLists.txt for this commit

@bader bader requested a review from maarquitos14 November 20, 2023 16:21
@bader bader requested a review from steffenlarsen November 20, 2023 16:21
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

I have experimented locally with this PR and works perfectly. The improvement in compile time and runtime is very significant.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Great! 🚀

@bader
Copy link
Contributor

bader commented Nov 21, 2023

@keryell, all comments have been resolved. Could you take a look, please?

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.

Thanks!

@bader bader merged commit 3ace7fc into KhronosGroup:SYCL-2020 Nov 21, 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