[SYCL] Add prototype of ExtendedAtomics features #1826
Conversation
Features: - atomic_ref with integral and floating-point specializations - atomic_fence Tests: - exchange - compare_exchange - fetch_add, +=, ++ - fetch_sub, -=, -- - fetch_min - fetch_max Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Pennycook
added
the
spec extension
Pennycook
requested review from
AlexeySachkov,
jbrodman,
mkinsner and
a team
as code owners
|
@bader What version of clang-format should I be using now? Is there a way to check that I'm using the right one? My local version (built from intel/llvm) doesn't format the same way as clang-format-patch. |
|
CI check is using clang-format-9. |
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Perfect, thanks! Two of the atomic tests are failing with the CUDA backend right now, so I've marked them as XFAIL. |
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
…ototype Signed-off-by: John Pennycook <[email protected]>
|
We need more internal investigation to work out why the accelerator target is failing. I've removed it from the tests for now and marked the extension as CPU- and GPU- only, to unblock this PR and enable people to experiment with it on the systems where it works as intended. |
|
@AlexeySachkov, @brodman, @againull: I expect all the tests to pass with this latest version, so it should be ready for review. |
|
@jbrodman, @mkinsner, @AlexeySachkov, @intel/llvm-reviewers-runtime, please, review. |
|
@Pennycook , I am very sorry for being so late with review. |
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
| @@ -155,6 +155,7 @@ extern SYCL_EXTERNAL TempRetT __spirv_ImageSampleExplicitLod(SampledType, | |||
| macro(__attribute__((opencl_local)), Arg) | |||
|
|
|||
| __SPIRV_ATOMICS(__SPIRV_ATOMIC_FLOAT, float) | |||
| __SPIRV_ATOMICS(__SPIRV_ATOMIC_FLOAT, double) | |||
There was a problem hiding this comment.
Just a question: Is 'half' type expected to be supported too eventually?
There was a problem hiding this comment.
Good question. OpenCL only defines atomics for 32- and 64-bit types, which is what I've started with here.
C++ allows an atomic_ref to be constructed from any TriviallyCopyable type, and I think eventually that's where we'd like to end up. Whether the atomic_ref for a given type is implemented on top of native instructions, a compare-exchange loop, a global lock, etc, would then be implementation-defined and device-specific.
We haven't yet defined exactly what sort of device queries we'll want to support for testing atomic functionality. If you have any ideas for what the feature sets should look like, or if there are any queries that would be useful for implementing reductions, please let me know!
|
This PR requires approval from @AlexeySachkov and @mkinsner as code-owners. |
|
I should be able to approve for @mkinsner. |
|
@bader - how can we get this out of review hell? |
|
@AlexeySachkov, could you take a look, please? |
|
Thank you. Please merge ASAP. |
I think we should run the tests again. This PR was tested 9 days ago. I'd to make sure it still passes the tests. |
Makes sense. |
Features:
Tests:
Signed-off-by: John Pennycook [email protected]
Co-authored-by: Alexey Sachkov [email protected]
Co-authored-by: Roland Schulz [email protected]