Skip to content

Conversation

Pennycook
Copy link
Contributor

Enables partial specialization of atomic_ref for pointer types.

Implementation assumes that both host and device pointers can be stored
in a uintptr_t, but uses compare_exchange to implement pointer arithmetic
rather than make assumptions about how pointers will be represented on
different devices.

Signed-off-by: John Pennycook [email protected]


There are a few changes in the tests that may not be obvious to reviewers, so capturing those here:

  • std::numeric_limits<T>::max() isn't defined for T*, but tests only ever required that the sentinel value was outside the range of the group.
  • Some tests started complaining about casts from int to T*, and the simplest fix was to use size_t to store the work-item ID in those tests (since it's defined to be a size_t anyway).
  • The min and max tests mistakenly included comments referencing the pointer specialization, so I removed them -- not adding fetch_min and fetch_max to the pointer specialization of atomic_ref is deliberate

Enables partial specialization of atomic_ref for pointer types.

Implementation assumes that both host and device pointers can be stored
in a uintptr_t, but uses compare_exchange to implement pointer arithmetic
rather than make assumptions about how pointers will be represented on
different devices.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook added enhancement New feature or request spec extension All issues/PRs related to extensions specifications labels Jul 24, 2020
Triggers a pointer validity check that isn't required here.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook marked this pull request as ready for review July 27, 2020 18:21
@Pennycook Pennycook requested review from jbrodman, mkinsner and a team as code owners July 27, 2020 18:21
alexbatashev
alexbatashev previously approved these changes Jul 29, 2020
Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

LGTM

| [SYCL_INTEL_device_specific_kernel_queries](DeviceSpecificKernelQueries/SYCL_INTEL_device_specific_kernel_queries.asciidoc) | Proposal | |
| [SYCL_INTEL_enqueue_barrier](EnqueueBarrier/enqueue_barrier.asciidoc) | Supported(OpenCL, Level Zero) | |
| [SYCL_INTEL_extended_atomics](ExtendedAtomics/SYCL_INTEL_extended_atomics.asciidoc) | Partially supported(OpenCL: CPU, GPU) | Not supported: pointer types |
| [SYCL_INTEL_extended_atomics](ExtendedAtomics/SYCL_INTEL_extended_atomics.asciidoc) | Supported(OpenCL) | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean FPGAs also gain support for atomic_ref? Should we enable tests on FPGA emulator as well then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no. That's a mistake -- good catch. I should have changed it from "Partially supported" to "Supported", but left the set of supported platforms the same. Fixed in 0c19be3.

Should be fully supported, but only a subset of devices.

Signed-off-by: John Pennycook <[email protected]>
@bader bader merged commit a3c3425 into intel:sycl Jul 30, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jul 31, 2020
…rogram

* upstream/sycl:
  [SYCL] Add barrier before leader guard in LowerWGSCope pass (intel#2208)
  [SYCL] Add prototype of atomic_ref<T*> (intel#2177)
  Revert "[SYCL] Disallow mutable lambdas (intel#1785)" (intel#2213)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request spec extension All issues/PRs related to extensions specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants