8000 [SYCL] Fix `sycl::atomic` regression by AlexeySachkov · Pull Request #19015 · intel/llvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SYCL] Fix sycl::atomic regression #19015

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 25, 2025
Merged

[SYCL] Fix sycl::atomic regression #19015

merged 6 commits into from
Jun 25, 2025

Conversation

AlexeySachkov
Copy link
Contributor
@AlexeySachkov AlexeySachkov commented Jun 17, 2025 •
edited
Loading

This is a follow-up for #18839 which broke host compilation of sycl::atomic.

The actual fix is just an addition of #include <sycl/__spirv/spirv_ops.hpp> to sycl/atomic.hpp.

However, I didn't want spirv_ops.hpp to appear again in sycl/detail/core.hpp and therefore I went further to make sure that sycl/atomic.hpp isn't used by sycl/accessor.hpp which prompted other changes in this PR.

@AlexeySachkov
[SYCL] Fix sycl::atomic regression
17d0231 
This is a follow-up for intel#18839 which broke host compilation of
`sycl::atomic`.

The actual fix is just an addition of `#include
<sycl/__spirv/spirv_ops.hpp>` to `sycl/atomic.hpp`.

However, I didn't want `spirv_ops.hpp` appear again in `sycl/detail/core.hpp`
and therefore I went further to make sure that `sycl/atomic.hpp` isn't used by
`sycl/accessor.hpp` which prompted other changes in this PR.
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner June 17, 2025 09:58
@AlexeySachkov
Copy link
Contributor Author

Wow, that Windows issue is persistent

I honestly struggle to understand how this patch could cause it at all:

# .---command stderr------------
# | In file included from D:\github\_work\llvm\llvm\llvm\sycl\test-e2e\DeviceLib\exp\exp-std-complex-float-edge-cases.cpp:10:
# | D:\github\_work\llvm\llvm\llvm\sycl\test-e2e\DeviceLib\exp\exp-std-complex-edge-cases.hpp:24:29: error: constexpr variable 'testcases' must be initialized by a constant expression
# |    24 |   constexpr std::complex<T> testcases[] = {
# |       |                             ^             ~
# |    25 |       /* 0 */ std::complex<T>(1.e-6, 1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    26 |       /* 1 */ std::complex<T>(-1.e-6, 1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    27 |       /* 2 */ std::complex<T>(-1.e-6, -1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    28 |       /* 3 */ std::complex<T>(1.e-6, -1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    29 | 
# |    30 |       /* 4 */ std::complex<T>(1.e+6, 1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    31 |       /* 5 */ std::complex<T>(-1.e+6, 1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    32 |       /* 6 */ std::complex<T>(-1.e+6, -1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    33 |       /* 7 */ std::complex<T>(1.e+6, -1.e-6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    34 | 
# |    35 |       /* 8 */ std::complex<T>(1.e-6, 1.e+6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    36 |       /* 9 */ std::complex<T>(-1.e-6, 1.e+6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    37 |       /* 10 */ std::complex<T>(-1.e-6, -1.e+6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    38 |       /* 11 */ std::complex<T>(1.e-6, -1.e+6),
# |       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# |    39 | 
# | D:\github\_work\llvm\llvm\llvm\sycl\test-e2e\DeviceLib\exp\exp-std-complex-float-edge-cases.cpp:12:21: note: in instantiation of function template specialization 'test<float>' requested here
# |    12 | int main() { return test<float>(); }
# |       |                     ^
# | D:\github\_work\llvm\llvm\llvm\sycl\test-e2e\DeviceLib\exp\exp-std-complex-edge-cases.hpp:63:32: note: floating point arithmetic produces a NaN
# |    63 |       /* 32 */ std::complex<T>(NAN, NAN),
# |       |                                ^
# | C:\Program Files (x86)\Windows Kits\10\include\10.0.22[621](https://github.com/intel/llvm/actions/runs/15729393865/job/44326718876?pr=19015#step:27:636).0\ucrt\corecrt_math.h:94:39: note: expanded from macro 'NAN'
# |    94 | #define NAN        (-(float)(INFINITY * 0.0F))
# |       |                                       ^
# | 1 error generated.
# `-----------------------------
# error: command failed with exit status: 1

And I haven't been able to reproduce it so far, but I will make more attempts

@AlexeySachkov
Copy link
Contributor Author

I still haven't figured out what exactly happens here. From what I see, the way of how NAN is defined by the MSVC provided standard C++ headers changes depending on what we include and on order of those includes.

I will spend a little more time investigating that, but I'm not sure that it is worth blocking the PR by it (considering the impact of the issue I'm fixing vs the impact of this issue): there are known reports of this macro being completely broken, so I wouldn't be surprised if this is a one more bug in there.

My plan is to have #define NAN within affected tests to bypass the issue, but if I do so, I will make sure to record this issue to investigate it as a follow-up.

@AlexeySachkov AlexeySachkov mentioned this pull request Jun 24, 2025
Open
@AlexeySachkov
Copy link
Contributor Author

#19114 was submitted to record the issue, I'm making a few more local attempts to try and resolve it, but if they fail, I will just redefine NAN in the test myself

@bader
Copy link
Contributor
bader commented Jun 25, 2025

@AlexeySachkov, #18839 broke SYCL-CTS build more than a week ago. Considering that the fix takes so long, I would prefer you to revert #18839 and re-apply fixed version.

@KornevNikita
Copy link
Contributor

@bader I believe this one is ready to merge

@bader
Copy link
Contributor
bader commented Jun 25, 2025

@bader I believe this one is ready to merge

@KornevNikita, could you approve then, please?

@AlexeySachkov AlexeySachkov merged commit 3de0989 into intel:sycl Jun 25, 2025
25 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/fix-atomic-regression branch June 25, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KornevNikita KornevNikita KornevNikita approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexeySachkov @bader @KornevNikita @aelovikov-intel
0