Skip to content

[SYCL] Port to event free APIs for kernel submission #1829

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KepingYan
Copy link

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 10, 2025 07:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the SYCLHelpers utility to use the event-free experimental APIs (parallel_for, nd_launch, submit) instead of the older handler.parallel_for and queue.submit calls.

  • Replaced cgh.parallel_for<ker_t>(…) with ::sycl::ext::oneapi::experimental::parallel_for<ker_t>(…).
  • Swapped q.submit(cgf) for ::sycl::ext::oneapi::experimental::submit(q, cgf).
  • Updated all overloads to invoke nd_launch<ker_t> for nd_range‐based kernel launches.
Comments suppressed due to low confidence (2)

src/comm/SYCLHelpers.h:56

  • This new call to the experimental submit API introduces a different execution path. We should add or update unit/integration tests to cover the event-free submission semantics and ensure behavior remains correct.
  ::sycl::ext::oneapi::experimental::submit(q, cgf);

src/comm/SYCLHelpers.h:49

  • [nitpick] The function comment above this declaration still refers to the old submission model. Please update the doc comment to mention that it now uses the experimental event-free APIs (parallel_for, nd_launch, submit).
static inline void sycl_kernel_submit(

Comment on lines +54 to +56
::sycl::ext::oneapi::experimental::parallel_for<ker_t>(cgh, range, ker);
};
::sycl::ext::oneapi::experimental::submit(q, cgf);
Copy link
Preview

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

[nitpick] The fully qualified namespace ::sycl::ext::oneapi::experimental is repeated multiple times. Consider introducing a namespace alias (e.g., namespace sycl_exp = ::sycl::ext::oneapi::experimental;) to reduce verbosity and improve readability.

Suggested change
::sycl::ext::oneapi::experimental::parallel_for<ker_t>(cgh, range, ker);
};
::sycl::ext::oneapi::experimental::submit(q, cgf);
sycl_exp::parallel_for<ker_t>(cgh, range, ker);
};
sycl_exp::submit(q, cgf);

Copilot uses AI. Check for mistakes.

@gujinghui gujinghui requested a review from fengyuan14 July 10, 2025 14:05
@gujinghui
Copy link
Contributor

Should we add build assertion to check SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS first?

@KepingYan
Copy link
Author

Should we add build assertion to check SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS first?

Do we need to keep the original code path? If SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS is defined and set to 1, run the modified code; otherwise, use the original code. Or just add an assertion?

@gujinghui
Copy link
Contributor

Should we add build assertion to check SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS first?

Do we need to keep the original code path? If SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS is defined and set to 1, run the modified code; otherwise, use the original code. Or just add an assertion?

Should be no. According to previous discussion, we have to go to the event-less path, anyway.

@KepingYan
Copy link
Author

Gentle ping @fengyuan14 .

@guangyey guangyey requested a review from chuanqi129 July 17, 2025 10:53
@guangyey
Copy link
Contributor

Hi @chuanqi129, could you help assign the right person to validate whether this change may introduce any unintended performance regressions?

#if !defined(SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS) || \
(SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS != 1)
#error This change enables event-less APIs, \
please ensure SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS=1.
Copy link
Contributor

Choose a reason for hiding this comment

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

ifdef SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS is good enough, as the only value is 1 when defined.

BTW, can we provide more readable error message? For example,
FATAL ERROR: The SYCL_EXT_ONEAPI_ENQUEUE_FUNCTIONS is excepted to be defined to enqueue kernel without returning event. But the undef is detected!

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