Skip to content

[SYCL] Add tests for atomic operations on malloc_shared-allocated memory #12480

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 2 commits into from
Feb 29, 2024

Conversation

pasaulais
Copy link
Contributor

This duplicates existing SYCL atomic tests that use buffers with identical tests that use USM and memory allocated using sycl::malloc_shared. The motivation behind exercising atomics on malloc_shared-allocated memory is to catch issues such as ROCm/ROCm#848 where native atomic instructions may fail when crossing the PCIe bus.

@pasaulais pasaulais requested a review from a team as a code owner January 24, 2024 14:13
@pasaulais
Copy link
Contributor Author

This PR so far only contains changes to the 'add' atomic tests to get initial feedback. I will add other tests later so that review feedback won't potentially require all tests to be changed.

Copy link
Contributor

github-actions bot commented Jan 24, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@maarquitos14
Copy link
Contributor

This PR so far only contains changes to the 'add' atomic tests to get initial feedback. I will add other tests later so that review feedback won't potentially require all tests to be changed.

The changes so far look good to me. Only one nit: comments are supposed to be written in proper English, i.e. with proper punctuation. I noticed that you simply copy-pasted the comments from existing tests into the new tests, but can you add the . at the end of the sentence for each of them? Just for the newly added ones.

@pasaulais pasaulais force-pushed the usm-shared-atomic-tests branch from 732e589 to ecf148b Compare January 25, 2024 11:45
@pasaulais
Copy link
Contributor Author

This PR so far only contains changes to the 'add' atomic tests to get initial feedback. I will add other tests later so that review feedback won't potentially require all tests to be changed.

The changes so far look good to me. Only one nit: comments are supposed to be written in proper English, i.e. with proper punctuation. I noticed that you simply copy-pasted the comments from existing tests into the new tests, but can you add the . at the end of the sentence for each of them? Just for the newly added ones.

No problem. I've clang-formatted my changes and added . to the comments in the new code. I can do it for the existing code as well as I go. Is there any reason not to, like keeping the diff simple?

@maarquitos14
Copy link
Contributor

No problem. I've clang-formatted my changes and added . to the comments in the new code. I can do it for the existing code as well as I go. Is there any reason not to, like keeping the diff simple?

Yes, that's exactly it, we want to keep the diff simple.

@pasaulais
Copy link
Contributor Author

@maarquitos14 I have completed the tests for all atomic operations present in the test-e2e/AtomicRef folder, using the same approach as the 'add' test you've already reviewed. Could you take another look at it?

@pasaulais pasaulais force-pushed the usm-shared-atomic-tests branch from b85d9aa to 830c886 Compare February 22, 2024 12:58
@pasaulais
Copy link
Contributor Author

Thanks for your comments @maarquitos14, I think I've addressed all of them.

@pasaulais
Copy link
Contributor Author

pasaulais commented Feb 28, 2024

All atomic tests now seem to be failing on CI (AMD HIP only as far as I can see) with:

# | terminate called after throwing an instance of 'sycl::_V1::exception'
# |   what():  Device does not support shared USM allocations!

This seems to have been introduced by #12700. I'm not sure why this is not supported on AMD HIP, but either way the tests need to be modified to check for shared USM support.

@pasaulais pasaulais force-pushed the usm-shared-atomic-tests branch from 07ba3c6 to 2c391ba Compare February 29, 2024 12:16
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM.

@ldrumm ldrumm merged commit 42c0092 into intel:sycl Feb 29, 2024
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.

3 participants