Skip to content
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

Implement UniqueToken for SYCL #3833

Merged
merged 12 commits into from
Mar 18, 2021
Merged

Conversation

masterleinad
Copy link
Contributor

This also allows running all container tests.

AFAICT, the concurrency calculation here is an upper limit for Intel GPUs and should match the value we get for Nvidia GPUs. As usual, I checked that this pull request also passes on Intel GPUs.
We still don't have a good implementation for clock_tick so the UniqueToken might be pretty slow.

@masterleinad masterleinad requested a review from nliber March 9, 2021 15:36
@masterleinad masterleinad added this to the Tentative 3.4 Release milestone Mar 9, 2021
@masterleinad masterleinad marked this pull request as draft March 9, 2021 16:24
@masterleinad
Copy link
Contributor Author

Hmm.. This passes with an older SYCL-CUDA release but not with the one we are now using for testing. I'll try to check which one broke or at least if I can find one that we can compile and that works also for this pull request.

@masterleinad masterleinad force-pushed the sycl_uniquetoken branch 3 times, most recently from ea0bc9b to cb80bda Compare March 11, 2021 23:19
@masterleinad
Copy link
Contributor Author

This should work now. I'm still not quite sure about the internal compiler errors but they are related to files that shouldn't matter too much for the SYCL CI (except maybe for the header self-containment tests). I will, of course, try to dig a little more but I think the status here is acceptable for the release. As mentioned before, the tests are all passing without the workarounds for Host and Intel GPU; only the SYCL CUDA backend causes all the problems here.

One of the problematic files was TestHostBarrier.cpp which turned out to be empty. I'm happy to create another pull request for that.

Other than that, I needed to rebase on top of #3815 to make all tests pass.

@masterleinad masterleinad marked this pull request as ready for review March 11, 2021 23:22
@masterleinad
Copy link
Contributor Author

Retest this please.

Copy link
Member

@Rombur Rombur left a comment

Choose a reason for hiding this comment

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

This looks fine with the caveat that I don't know SYCL...

core/src/SYCL/Kokkos_SYCL_Parallel_Team.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Parallel_Team.hpp Outdated Show resolved Hide resolved
@masterleinad masterleinad added the Blocks Promotion Overview issue for release-blocking bugs label Mar 17, 2021
@masterleinad
Copy link
Contributor Author

Rebased. Still depends on #3815.

@dalg24
Copy link
Member

dalg24 commented Mar 18, 2021

Please rebase

@dalg24 dalg24 merged commit 2c69fa0 into kokkos:develop Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants