Skip to content

[SYCL] Bump native enqueue extension version #18321

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

Merged
merged 1 commit into from
May 14, 2025

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented May 5, 2025

In #16871 the sycl_ext_codeplay_enqueue_native_command extensions was extended to add new interop_handler APIs for working with SYCL-Graph.

However the extension macro was not bumped, which I think was an oversight. This is problematic for users that want to use the extension with graph support, but also use older oneAPI releases.

#ifdef SYCL_EXT_ONEAPI_ENQUEUE_NATIVE_COMMAND 
CGH.ext_codeplay_enqueue_native_command([=](sycl::interop_handle IH) {
    if (IH.ext_codeplay_has_graph()) {  // Is this API defined?
      // Graph path
    } else {
      // Eager path
    }
#else
CGH.host_task(...)
#endif

By bumping the feature test macro users can write code that supports old DPC++ versions.

#ifdef SYCL_EXT_ONEAPI_ENQUEUE_NATIVE_COMMAND 
CGH.ext_codeplay_enqueue_native_command([=](sycl::interop_handle IH) {
#if SYCL_EXT_ONEAPI_ENQUEUE_NATIVE_COMMAND > 1
    if (IH.ext_codeplay_has_graph()) {
      // Graph path
    } else
#endif
    {
      // Eager path
    }
#else
CGH.host_task(...)
#endif

@EwanC EwanC temporarily deployed to WindowsCILock May 5, 2025 16:35 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock May 5, 2025 17:00 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock May 5, 2025 17:00 — with GitHub Actions Inactive
@EwanC EwanC marked this pull request as ready for review May 6, 2025 08:00
@EwanC EwanC requested review from a team as code owners May 6, 2025 08:00
@EwanC EwanC requested a review from cperkinsintel May 6, 2025 08:00
@gmlueck
Copy link
Contributor

gmlueck commented May 6, 2025

We usually do not version the experimental extensions for a variety of reasons. (For example, experimental extensions often change in ways that are not strictly backward compatible, and versioning doesn't help in that case.) If you want to version the extension, that's OK, though. Do you have a specific user request for adding the versioning?

If you do add it, I suggest making it more clear about which APIs are added in version 2. Is it just the APIs documented in "SYCL-Graph Interaction"? If so, add a paragraph in that section stating that those APIs are available starting in version 2 of the specification.

@EwanC
Copy link
Contributor Author

EwanC commented May 6, 2025

We usually do not version the experimental extensions for a variety of reasons. (For example, experimental extensions often change in ways that are not strictly backward compatible, and versioning doesn't help in that case.) If you want to version the extension, that's OK, though. Do you have a specific user request for adding the versioning?

UXL oneMath uses the native-command extension, but doesn't take into account the command-groups being graph recorded, which I found via debugging a llama.cpp issue. I'm writing a patch to fix this by using the interop-handle graph APIs, but I didn't want to break building tip oneMath with the most recent oneAPI release (2025.1) which won't have these APIs yet. I've not got direct feedback from the maintainers on this though, so we could wait to see what they say before progressing this.

@EwanC
Copy link
Contributor Author

EwanC commented May 13, 2025

@gmlueck I've checked with the oneMath codeowner (uxlfoundation/oneMath#669 (review)) and this version bump would be required to integrate the patch using the new APIs without breaking the 2025.1 build.

If you do add it, I suggest making it more clear about which APIs are added in version 2. Is it just the APIs documented in "SYCL-Graph Interaction"? If so, add a paragraph in that section stating that those APIs are available starting in version 2 of the specification.

Good idea, I've added that now

@EwanC EwanC temporarily deployed to WindowsCILock May 13, 2025 08:39 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock May 13, 2025 08:39 — with GitHub Actions Inactive
In intel#16871 the
`sycl_ext_codeplay_enqueue_native_command` extensions was extended to
add new `interop_handler` APIs for working with SYCL-Graph.

However, the extension macro was not bumped. This is problematic for
users that want to use the extension with graph support, but also
use older oneAPI releases.

```cpp
CGH.ext_codeplay_enqueue_native_command([=](sycl::interop_handle IH) {
    if (IH.ext_codeplay_has_graph() /* is this defined?*/) {
      // Graph path
    } else {
      // Eager path
    }
CGH.host_task(...)
```

By bumping the feature test macro, users can write code that supports
old DPC++ versions.
```cpp
CGH.ext_codeplay_enqueue_native_command([=](sycl::interop_handle IH) {

    if (IH.ext_codeplay_has_graph() /* is this defined?*/) {
      // Graph path
    } else
    {
      // Eager path
    }
CGH.host_task(...)
```
@EwanC EwanC force-pushed the ewan/bump_native-command_version branch from 5e9577a to f9382a2 Compare May 13, 2025 14:22
@EwanC EwanC temporarily deployed to WindowsCILock May 13, 2025 14:22 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock May 13, 2025 16:27 — with GitHub Actions Inactive
@EwanC EwanC temporarily deployed to WindowsCILock May 13, 2025 16:27 — with GitHub Actions Inactive
@EwanC
Copy link
Contributor Author

EwanC commented May 14, 2025

The CI failures are known issues with Arc #18416

Failed Tests (4):
  SYCL :: KernelAndProgram/cache_env_vars.cpp
  SYCL :: KernelAndProgram/cache_env_vars_lin.cpp
  SYCL :: KernelCompiler/opencl.cpp
  SYCL :: KernelCompiler/sycl_cache_pm.cpp

@intel/llvm-gatekeepers Can we merge this please

@martygrant martygrant merged commit 255c3f1 into intel:sycl May 14, 2025
24 of 25 checks passed
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.

4 participants