Skip to content

[SYCL][NFCI] Move abs and div to cstdlib header #19671

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 8 commits into from
Aug 4, 2025
Merged

Conversation

bader
Copy link
Contributor

@bader bader commented Jul 31, 2025

The C++ specification defines that abs(<integer>) and div functions are provided by cstdlib.
Whether they're available or not after #include <cmath> is an implementation detail of the underlying C++ standard library implementation.

With this change we move the definition to the correct wrapper file
aligned with the C++ specification.

These functions are declared in cmath and use data type defined in
cstdlib. Some cmath implementations include cstdlib implicitly, but
not all of them. Moving declarations to a standalone header removes
dependency on cmath implementation.
bader added 2 commits July 31, 2025 15:40
Remove std::div and std::abs overloads and remove xfail mark for the
test.

Align cstdlib declaration with cppreference:
1. Make all declarations overloadable.
2. Fix function name for div(long long, long long) overload and add
   missing overload.
3. Add std::* variants.
@bader bader marked this pull request as ready for review July 31, 2025 23:20
@bader bader requested a review from a team as a code owner July 31, 2025 23:20
@bader
Copy link
Contributor Author

bader commented Aug 1, 2025

run_prebuilt_e2e_tests (E2E tests on Intel Ponte Vecchio GPU, ["Linux", "pvc"], -u 1001 --device=... / E2E tests on Intel Ponte Vecchio GPU failure doesn't seem to be related to the change. The test doesn't use abs or div functions and there is known issue with this test on PVC. See #19662.

@jinge90
Copy link
Contributor

jinge90 commented Aug 1, 2025

Hi, @bader
Can we keep the functions in libdevice? They are used by OMP offloading as well.
If can't, we can only build them for OMP offloading side.
Thanks very much.

@bader
Copy link
Contributor Author

bader commented Aug 1, 2025

Can we keep the functions in libdevice? They are used by OMP offloading as well. If can't, we can only build them for OMP offloading side. Thanks very much.

There is no sense in making any changes for OpenMP offloading in this repository because OpenMP offloading is not supported/tested here.

In the upstream OpenMP offloading uses clang's headers to implement abs on AMD GPU. I suggest using the same approach for SPIR-V targets in other repositories as well.
The same applies to div if it's needed for OpenMP offloading.

@sarnex, are you working on something similar in the upstream?

Copy link
Contributor

@Maetveis Maetveis left a comment

Choose a reason for hiding this comment

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

This will conflict with #19620. Can you apply the changes here I did to these functions, I don't want it to be lost during conflict resolution.

@sarnex
Copy link
Contributor

sarnex commented Aug 1, 2025

@sarnex, are you working on something similar in the upstream?

I haven't begun to think about how we will deal with libdevice in OMP upstream, however I agree we should match whatever the community does as best as possible.

@bader bader temporarily deployed to WindowsCILock August 1, 2025 20:39 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock August 1, 2025 20:39 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock August 2, 2025 02:02 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock August 2, 2025 02:30 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock August 2, 2025 02:30 — with GitHub Actions Inactive
Copy link
Contributor

@Maetveis Maetveis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bader
Copy link
Contributor Author

bader commented Aug 4, 2025

@intel/llvm-reviewers-runtime, ping.

@aelovikov-intel
Copy link
Contributor

These functions are declared in cmath and use data type defined in
cstdlib.

This is ambiguous as cmath/cstdlib might refer to either system implementation or our "wrappers". Also, if they do refer to our wrappers, then it should have been something like "were declared in our cmath wrapper before this PR".

It seems you're trying to say something like

These functions are only guaranteed to be provided by `cstdlib` 
by the specification and whether they're available or not after 
`#include <cmath>` is an implementation detail of the underlying 
libc implementation.

The fact that we've been providing them as part of our `cmath` wrapper
was a hack (see pre-existing comments in <insert_file>) prior to this PR.
With this change we move the definition to the correct wrapper file
aligned with the specification/host libc implementation/<insert proper term>.

@bader
Copy link
Contributor Author

bader commented Aug 4, 2025

@aelovikov-intel, thanks. I've updated the pull request description. Please, take a look.

@bader bader merged commit b53f646 into intel:sycl Aug 4, 2025
26 checks passed
@bader bader deleted the cstdlib branch August 4, 2025 22:28
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