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

[SYCL][HIP] Implement PI_DEVICE_INFO_ATOMIC_64 for HIP #6429

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

pgorlani
Copy link
Contributor

This patch implements aspect::atomic64 by querying the information related to the 64-bit integer atomic operations. At the moment, the floating-point atomics are not supported at all (single and double precision) for AMD devices. The SYCL_USE_NATIVE_FP_ATOMICS is not set for AMD, and the atomc_ref class handles the lack of double and single precision atomic operations.

We may want to revisit this implementation when the floating-point atomics will be supported by AMD too.

However, I think this is not necessary for three reasons:

  1. the atomic64 aspect "Indicates that kernels submitted to the device may perform 64-bit atomic operations." It expresses a possibility, not specifying any kind of operations or data types.
  2. the SYCL specifications for atomic_ref say "For floating-point types, the member functions of the atomic_ref class may be emulated, and may use a different floating-point environment to ..." implying that the floating-point operations in atomic_ref do not need to be supported natively by the hardware.
  3. both L0 and OpenCL backends check only for 64-bit integer atomics.

This solves #5570, #6054, and allows to re-enable tests in intel/llvm-test-suite#861.

@pgorlani pgorlani requested a review from a team as a code owner July 12, 2022 12:11
@sergey-semenov
Copy link
Contributor

@pgorlani The changes looks good, but there are now unexpected passes in testing. Is there an llvm-test-suite PR for enabling those two tests?

pgorlani added a commit to pgorlani/llvm-test-suite that referenced this pull request Jul 13, 2022
This patch re-enables the aspect::atomic64 tests for
HIP after the PI_DEVICE_INFO_ATOMIC_64 implementation
in intel/llvm#6429
@pgorlani
Copy link
Contributor Author

Hi @sergey-semenov, here is intel/llvm-test-suite#1094.

@sergey-semenov
Copy link
Contributor

/verify with intel/llvm-test-suite#1094

@pgorlani
Copy link
Contributor Author

Hi @sergey-semenov,
is it possible to merge this PR?

Thanks!

@againull againull merged commit cb190fc into intel:sycl Aug 12, 2022
againull pushed a commit to intel/llvm-test-suite that referenced this pull request Aug 12, 2022
This patch re-enables the aspect::atomic64 tests for
HIP after the PI_DEVICE_INFO_ATOMIC_64 implementation
in intel/llvm#6429
AlexeySachkov added a commit to AlexeySachkov/llvm-test-suite that referenced this pull request Sep 7, 2022
Support for atomic64 aspect query on HIP was added in:

commit cb190fc25d56f42b3283a3a31c13ead47be46f60
Author: pgorlani <92453485+pgorlani@users.noreply.github.com>
Date:   Fri Aug 12 18:54:29 2022 +0100

    [SYCL][HIP] Implement PI_DEVICE_INFO_ATOMIC_64 for HIP (intel/llvm#6429)
AlexeySachkov added a commit to intel/llvm-test-suite that referenced this pull request Sep 7, 2022
Support for atomic64 aspect query on HIP was added in:

```
commit cb190fc25d56f42b3283a3a31c13ead47be46f60
Author: pgorlani <92453485+pgorlani@users.noreply.github.com>
Date:   Fri Aug 12 18:54:29 2022 +0100

    [SYCL][HIP] Implement PI_DEVICE_INFO_ATOMIC_64 for HIP (intel/llvm#6429)
```
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
)

This patch re-enables the aspect::atomic64 tests for
HIP after the PI_DEVICE_INFO_ATOMIC_64 implementation
in intel#6429
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
Support for atomic64 aspect query on HIP was added in:

```
commit cb190fc
Author: pgorlani <92453485+pgorlani@users.noreply.github.com>
Date:   Fri Aug 12 18:54:29 2022 +0100

    [SYCL][HIP] Implement PI_DEVICE_INFO_ATOMIC_64 for HIP (intel#6429)
```
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