Skip to content

[SYCL][Devicelib] Implement cmath rintf wrapper with __spirv_ocl_rint #18857

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 3 commits into from
Jun 10, 2025

Conversation

wenju-he
Copy link
Contributor

@wenju-he wenju-he commented Jun 9, 2025

This PR is to support the use of std::rint in device code. Currently it
is resolved to rintf symbol. With this PR, the rintf symbol is resolved
by libdevice.

@wenju-he wenju-he requested a review from a team as a code owner June 9, 2025 07:48
@wenju-he wenju-he requested a review from againull June 9, 2025 07:48
@wenju-he wenju-he temporarily deployed to WindowsCILock June 9, 2025 07:48 — with GitHub Actions Inactive
@wenju-he wenju-he requested a review from jinge90 June 9, 2025 07:48
@wenju-he wenju-he temporarily deployed to WindowsCILock June 9, 2025 08:07 — with GitHub Actions Inactive
@wenju-he wenju-he temporarily deployed to WindowsCILock June 9, 2025 08:07 — with GitHub Actions Inactive
Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

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

LGTM, we could probably remove the NVPTX/AMDGCN specific path and use the __spirv_ocl_rint path for them as well, but we can look at that in a different patch.

@bader
Copy link
Contributor

bader commented Jun 9, 2025

In general, there is no point in adding __device_lib_<func> fallback implementation to the devicelib for the functions which have corresponding __spirv_ocl_<func>. The whole idea behind __device_lib_ is to bypass adding SPIR-V extensions for C/C++ standard library functions. If SPIR-V instruction is already defined, we can implement corresponding C/C++ functions as wrappers in the headers.

Copy link
Contributor

@againull againull left a comment

Choose a reason for hiding this comment

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

Could you add a test please.

@wenju-he
Copy link
Contributor Author

If SPIR-V instruction is already defined, we can implement corresponding C/C++ functions as wrappers in the headers.

Could you point me to the wrapper that implements std::rint with __spirv_ocl_rint?

This PR is to support use of std::rint in device code. Currently it is resolved to rintf symbol. With this PR, rintf symbol is resolved by libdevice.

@jinge90
Copy link
Contributor

jinge90 commented Jun 10, 2025

If SPIR-V instruction is already defined, we can implement corresponding C/C++ functions as wrappers in the headers.

Could you point me to the wrapper that implements std::rint with __spirv_ocl_rint?

This PR is to support use of std::rint in device code. Currently it is resolved to rintf symbol. With this PR, rintf symbol is resolved by libdevice.

Hi, @bader and @wenju-he
If it is for support C++ std::rint, the header file is in user's system header which we can't modify. But we have decided to remove fallback devicelib, we can directly use __spirv_oc_rint in cmath_wrapper.cpp 'rintf' function.

@wenju-he wenju-he changed the title [SYCL][Devicelib] Add __devicelib_rint and fallback to __spirv_ocl_rint [SYCL][Devicelib] Implement cmath rintf wrapper with __spirv_ocl_rint Jun 10, 2025
@wenju-he
Copy link
Contributor Author

the header file is in user's system header which we can't modify. But we have decided to remove fallback devicelib, we can directly use __spirv_oc_rint in cmath_wrapper.cpp 'rintf' function.

done, thanks.

@npmiller
Copy link
Contributor

If SPIR-V instruction is already defined, we can implement corresponding C/C++ functions as wrappers in the headers.

Could you point me to the wrapper that implements std::rint with __spirv_ocl_rint?

This PR is to support use of std::rint in device code. Currently it is resolved to rintf symbol. With this PR, rintf symbol is resolved by libdevice.

It's not actually ready yet, but I've been working on that in #18706 which will address this rint issue once completed, but it needs more work and is currently only tested for CUDA/HIP. So I think it's fine to go ahead with this PR until the header solution is ready.

@againull againull merged commit fbf735a into intel:sycl Jun 10, 2025
24 checks passed
@wenju-he wenju-he deleted the __devicelib_rint branch June 11, 2025 01:21
@wenju-he
Copy link
Contributor Author

It's not actually ready yet, but I've been working on that in #18706 which will address this rint issue once completed, but it needs more work and is currently only tested for CUDA/HIP. So I think it's fine to go ahead with this PR until the header solution is ready.

thanks @npmiller, #18706 looks great.

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.

5 participants