Skip to content

[SYCL][CUDA] libclc atomics update #2100

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
Closed
wants to merge 8 commits into from
Closed

[SYCL][CUDA] libclc atomics update #2100

wants to merge 8 commits into from

Conversation

Alexander-Johnston
Copy link
Contributor

Updates the libclc spirv atomics used by the sycl cuda backend.

Includes

  • Improvements to the bindings between libspirv and sycl for cuda
  • Additional atomics
  • Better testing in libclc for these atomics

@Alexander-Johnston Alexander-Johnston force-pushed the Alex/libclc-atomics-update branch from 7dd3aeb to 49e52be Compare July 13, 2020 14:36
Pennycook
Pennycook reviewed Jul 13, 2020
View reviewed changes
@Alexander-Johnston Alexander-Johnston force-pushed the Alex/libclc-atomics-update branch from 49e52be to 6175e23 Compare July 13, 2020 16:17
Alexander Johnston and others added 7 commits July 14, 2020 13:49
@Naghasan
Remove data layout requirements from libclc atomics
eacbcd8 
Authored-by: Stuart Adams <[email protected]>
Co-authored-by: Victor Lomuller <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
Update SPIRVBuiltins.td for libclc OpenCL atomics
849f145 
Authored-by: Victor Lomuller <[email protected]>
Co-authored-by: Alexander Johnston <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
Use an include list for libclc test generation
32e78b0 
Authored-by: Victor Lomuller <[email protected]>
Co-authored-by: Alexander Johnston <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
Add libclc binding tests for atomics
4cd7963 
Authored-by: Victor Lomuller <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
Update generated spirv_builtins.h
fefb629 
Authored-by: Victor Lomuller <[email protected]>
Co-authored-by: Alexander Johnston <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
@nyalloc
Remove manual mangling of libspirv atomics
8d772ac 
Authored-by: Alexander Johnston <[email protected]>
Co-authored-by: Stuart Adams <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
Update sycl use of libclc atomics for CUDA
f04e389 
Authored-by: Stuart Adams <[email protected]>
Co-authored-by: Alexander Johnston <[email protected]>
Signed-off-by: Alexander Johnston <[email protected]>
@Alexander-Johnston Alexander-Johnston force-pushed the Alex/libclc-atomics-update branch from 6175e23 to f04e389 Compare July 14, 2020 13:02
@bader bader requested a review from Pennycook July 15, 2020 07:58
Pennycook
Pennycook requested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

I still think adding a comment to spirv.hpp would be very helpful (see https://github.com/intel/llvm/pull/2100/files#r453772103).

@bader bader added the cuda CUDA back-end label Jul 15, 2020
@bader bader requested a review from Pennycook July 16, 2020 10:30
@elizabethandrews
Copy link
Contributor

Not really familiar with this. @premanandrao @erichkeane can you take a look?

@bader
Copy link
Contributor

bader commented Jul 17, 2020

@Alexander-Johnston, it looks like some functionality enabled by this patch is not supported by Intel OpenCL CPU device. Could you mark regression/implicit_atomic_conversion.cpp as expected to fail on CPU and ACC devices to unblock the merge, please?
We can investigate this issue later.

@Pennycook
Copy link
Contributor

@bader: I don't think it's this patch's fault. That regression test is for the functionality introduced by #2044, and #2127 is going to stop that functionality from being the default.

@bader
Copy link
Contributor

bader commented Jul 18, 2020

@Pennycook: #2127 didn't help. The test is still failing.

@MrSidims
Copy link
Contributor

@Pennycook: #2127 didn't help. The test is still failing.

I'll take a look.

@MrSidims
Copy link
Contributor

MrSidims commented Jul 18, 2020 •
edited
Loading

@Pennycook: #2127 didn't help. The test is still failing.

I'll take a look.

Purpose of the test is to check if compilation of a code with atomics that has implicit conversions for a fresh-new address spaces is working correct, so since the test in this PR is failing in runtime - #2127 shall has no affect on it. Also I don't mind to disable runs of the test on CPU/GPU. But the thing that worries me is that this change might break atomic conformance tests. Are we considering to deprecate atomics right at this point of time?

@bader
Copy link
Contributor

bader commented Jul 18, 2020

/summary:run

@bader
Copy link
Contributor

bader commented Jul 20, 2020

But the thing that worries me is that this change might break atomic conformance tests.

@MrSidims, is right. This change breaks test_atomic from Khronos SYCL-CTS.

@romanovvlad
Copy link
Contributor

Please, fix tests and reopen the PR.

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@callumfare
Merge pull request #2100 from kbenzie/benie/windows-warning-level-4
a13fc7e 
Raise MSVC warning level from /W3 to /W4
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@callumfare
Merge pull request #2302 from callumfare/callum/revert_ur_2100
2b3aaab 
Revert #2100 "Raise MSVC warning level from /W3 to /W4"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalloc nyalloc nyalloc left review comments

@bader bader bader approved these changes

@Pennycook Pennycook Pennycook approved these changes

@premanandrao premanandrao premanandrao approved these changes

@againull againull againull approved these changes

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

@elizabethandrews elizabethandrews Awaiting requested review from elizabethandrews

@Fznamznon Fznamznon Awaiting requested review from Fznamznon

Assignees
No one assigned
Labels
cuda CUDA back-end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Alexander-Johnston @elizabethandrews @bader @Pennycook @MrSidims @romanovvlad @nyalloc @premanandrao @againull