Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Updated atomic_memory_order_capabilities query test for OpenCL 3.0 backend #1627

Open
wants to merge 7 commits into
base: intel
Choose a base branch
from

Conversation

andylshort
Copy link

Both context and device should return at least memory_order::relaxed now on implementation-side in intel/llvm#8517, so this test should pass if the target device has an OpenCL 3.0 backend or newer.

Remaining work/discussions required:

  1. How should the test be structured for Level Zero and OpenCL <3.0 backends? These don't support the query presently (subject to change?) so this test can fail on older OCL backends currently.
  2. atomic_memory_order_acq_rel.cpp and atomic_memory_order_seq_cst.cpp require updating once the query fully supports the rest of the possible values, also.

Both context and device should return at least memory_order::relaxed now on
implementation-side, so this test should pass if the target device has
an OpenCL 3.0 backend or newer.
@andylshort
Copy link
Author

/verify with intel/llvm#8517

Need to adjust figures in acq_rel test to reduce work-group size so test
can be used with all backends and devices.
@andylshort andylshort marked this pull request as ready for review March 20, 2023 14:46
@andylshort andylshort requested a review from a team as a code owner March 20, 2023 14:46
@andylshort andylshort requested a review from againull March 20, 2023 14:46
@andylshort
Copy link
Author

One final question to clarify something: The acq_rel test, as it stands (I've not modified the functionality, just added the level-zero and OpenCl backends to check) will fail if the target OCL backend implementation is <2.0 but should work otherwise. I've asked around and there doesn't yet seem to be a way to test or filter out certain OpenCL versions, but don't want to leave it untested for relevant/newer implementations.

Is it fine to merge as it stands? Would it affect the CI? I'm not sure what version of OpenCL the runners use.

AlexeySachkov pushed a commit to intel/llvm that referenced this pull request Mar 23, 2023
…8517)

This patch implements the `atomic_memory_order_capabilities` query in
the OpenCL and Level Zero backends/plugins for `device` and `context`

Specifically: 
- OpenCL <2.0 returns the minimum required capability set (`relaxed`)
defined in [Section 4.2 of the OpenCL 3.0
specification](https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_API.html#CL_DEVICE_ATOMIC_MEMORY_CAPABILITIES).
- OpenCL <3.0 and Level Zero backends return all memory order
capabilities.
- OpenCL >=3.0 queries the actual device to get the supported memory
order capabilities.

E2E test have also been updated to reflect these changes:
intel/llvm-test-suite#1627
Lamzed-Short, Andrew added 3 commits March 23, 2023 06:55
Passes on cpu, fails on GPU, so I think a problem exists with the GPU driver
implementation, maybe specifically with fetch_add.
@andylshort
Copy link
Author

I've reduced the work group size for the acq_rel test. It works now, but still fails on GPU targets. I believe there may be an issue in the driver implementation for GPUs - on both OpenCl and Level Zero. Likely with fetch_add or atomic_ref in general.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants