llvm
llvm copied to clipboard
intel
[SYCL] Add tests for atomic operations on malloc_shared-allocated memory
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 https://github.com/ROCm/ROCm/issues/848 where native atomic instructions may fail when crossing the PCIe bus.
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.
:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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?
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.
@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?
Thanks for your comments @maarquitos14, I think I've addressed all of them.
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 https://github.com/intel/llvm/pull/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.