Skip to content

[SYCL] Refactor cmath wrapper templates #19483

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 2 commits into from
Jul 17, 2025

Conversation

npmiller
Copy link
Contributor

@npmiller npmiller commented Jul 16, 2025

Follow up from: #18706

  • Include <type_traits>, this was working because <cmath> pulls it in but this is cleaner.
  • Use C++14 and C++17 _v and _t helpers.
  • Switch to variadic template for __sycl_promote, using C++17 fold expressions. In theory only needed for 1 to 3 types, but this is an internal helper anyway so it doesn't seem worth restricting.
  • Switch typedef to using

With this patch the header now require C++17 support to compile. Which is fine since it's the minimum requirement for SYCL 2020, and this header is only compiled on the device side which is always handled by clang, so there shouldn't be any support issues.

* Include `<type_traits>`, this was working because `<cmath>` pulls it
  in but this is cleaner.
* Use C++14 and C++17 `_v` and `_t` helpers.
* Switch to variadic template for `__sycl_promote`, using C++17 fold
  expressions.

With this patch the header now require C++17 support to compile. Which
is fine since it's the minimum requirement for SYCL 2020, and this
header is only compiled on the device side which is always handled by
clang, so there shouldn't be any support issues.
@npmiller npmiller requested a review from a team as a code owner July 16, 2025 16:48
@npmiller npmiller requested a review from maarquitos14 July 16, 2025 16:48
@npmiller npmiller requested a review from aelovikov-intel July 16, 2025 16:49
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

LGTM, but if you don't mind doing another iteration here, please change

typedef __sycl_promote_t<..> type; to using type = __sycl_promote_t<..>;

@npmiller
Copy link
Contributor Author

Good shout, the using notation is nicer, I've just updated the patch with it.

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Thanks!

@npmiller
Copy link
Contributor Author

@intel/llvm-gatekeepers this is ready to merge

@sommerlukas sommerlukas merged commit 281f8e1 into intel:sycl Jul 17, 2025
25 checks passed
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.

3 participants