Skip to content

[SYCL] Native host task submission without thread pool.#22518

Draft
slawekptak wants to merge 7 commits into
intel:syclfrom
slawekptak:host_tasks_native_scheduler
Draft

[SYCL] Native host task submission without thread pool.#22518
slawekptak wants to merge 7 commits into
intel:syclfrom
slawekptak:host_tasks_native_scheduler

Conversation

@slawekptak

Copy link
Copy Markdown
Contributor

A change to the native host task submission logic, to submit the host tasks to UR without the need for a separate host task thread.

The host tasks no longer cause dependent commands to be queued in the scheduler.

A change to the native host task submission logic, to submit
the host tasks to UR without the need for a separate host task
thread.

The host tasks no longer cause dependent commands to be queued
in the scheduler.
which allows to implement a special event create path for the
native host task.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 SYCL host task submission to use Unified Runtime native host task support when available (removing reliance on a separate host-task thread pool) and adjusts scheduler/event plumbing to accommodate a new CGType::NativeHostTask.

Changes:

  • Introduces CGType::NativeHostTask and routes eligible host tasks to urEnqueueHostTaskExp.
  • Refactors ExecCGCommand event creation to better match host-task/native-host-task requirements.
  • Adds/updates unit and e2e tests to validate native-host-task behavior and native graph recording restrictions.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
sycl/unittests/Extensions/FreeFunctionCommands/FreeFunctionEventsHelpers.hpp Adds UR mock callbacks/counters for device info and native host task enqueue.
sycl/unittests/Extensions/EnqueueFunctionsEvents.cpp Adds tests covering native-host-task vs thread-pool host-task paths.
sycl/test-e2e/Graph/RecordReplay/NativeRecording/exception_host_task.cpp Adds coverage ensuring host_task is rejected in native recording mode.
sycl/source/handler.cpp Selects NativeHostTask vs CodeplayHostTask based on UR device capability query.
sycl/source/detail/scheduler/scheduler.cpp Updates scheduler CG handling and contains updated enqueue-failure error path.
sycl/source/detail/scheduler/commands.hpp Adds Command ctor overload and ExecCGCommand::makeEvent declaration.
sycl/source/detail/scheduler/commands.cpp Implements native host task enqueue path and refactors event creation for ExecCGCommand.
sycl/source/detail/host_task.hpp Simplifies HostTask origin tracking and updates constructors.
sycl/source/detail/graph/node_impl.hpp Excludes NativeHostTask from CG copying for graph nodes.
sycl/include/sycl/detail/cg_types.hpp Adds NativeHostTask to the CG type enum.

Comment on lines +31 to +33
HostTask() : MHostTask([]() {}) {}
HostTask(std::function<void()> &&Func) : MHostTask(Func) {}
HostTask(std::function<void(interop_handle)> &&Func) : MInteropTask(Func) {}
Comment thread sycl/source/handler.cpp
Comment on lines +1695 to +1705
detail::adapter_impl &Adapter = getContextImpl().getAdapter();
bool NativeHostTaskSupport = false;

Adapter.call<UrApiKind::urDeviceGetInfo>(
detail::getSyclObjImpl(Queue->get_device())->getHandleRef(),
UR_DEVICE_INFO_ENQUEUE_HOST_TASK_SUPPORT_EXP,
sizeof(NativeHostTaskSupport), &NativeHostTaskSupport, nullptr);
if (NativeHostTaskSupport) {
setType(detail::CGType::NativeHostTask);
return;
}
Comment on lines 199 to 207
throw sycl::detail::set_ur_error(
sycl::exception(
sycl::make_error_code(errc::runtime),
std::string("Enqueue process failed.\n") +
// TODO native host tasks - getWorkerContext() will return
// nullptr for native host task
__SYCL_UR_ERROR_REPORT(
NewCmd->getWorkerContext()->getBackend()) +
sycl::detail::codeToString(Res.MErrCode)),

@iclsrc iclsrc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This PR introduces a new NativeHostTask CG type that bypasses the SYCL thread-pool and submits host tasks directly via urEnqueueHostTaskExp, detected at submit time in SetHostTaskFromExtEnqueueFunctions. The refactoring also moves event construction into a new makeEvent helper called before the Command base is initialized.

Overall the approach is clean, but there is one critical crash risk in the error path (the acknowledged TODO on getWorkerContext()), a performance regression in the HostTask constructors (missing std::move), and a design risk with makeEvent being non-static.

Comment on lines +203 to 206
// TODO native host tasks - getWorkerContext() will return
// nullptr for native host task
__SYCL_UR_ERROR_REPORT(
NewCmd->getWorkerContext()->getBackend()) +

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 Critical: Guaranteed null dereference on NativeHostTask enqueue failure

The TODO comment on lines 203-204 explicitly documents that getWorkerContext() returns nullptr for a NativeHostTask command, yet line 206 calls ->getBackend() on that null pointer with no guard. If urEnqueueHostTaskExp ever fails (UR error, missing adapter support at runtime, etc.) this will crash.

The fix should guard the getWorkerContext() call before dereferencing:

Suggested change
// TODO native host tasks - getWorkerContext() will return
// nullptr for native host task
__SYCL_UR_ERROR_REPORT(
NewCmd->getWorkerContext()->getBackend()) +
std::string("Enqueue process failed.\n") +
(NewCmd->getWorkerContext()
? __SYCL_UR_ERROR_REPORT(
NewCmd->getWorkerContext()->getBackend())
: std::string()) +

Alternatively, give NativeHostTask commands a non-null worker context (e.g. the queue's context) so the common error path works without special-casing.

Comment on lines +32 to +33
HostTask(std::function<void()> &&Func) : MHostTask(Func) {}
HostTask(std::function<void(interop_handle)> &&Func) : MInteropTask(Func) {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Important: std::move missing — constructors copy instead of move

Both constructors accept rvalue references (&&) but then copy-construct the std::function members. This silently defeats the move semantics, especially costly when the captured lambda is large. The original code used std::move here.

Suggested change
HostTask(std::function<void()> &&Func) : MHostTask(Func) {}
HostTask(std::function<void(interop_handle)> &&Func) : MInteropTask(Func) {}
HostTask(std::function<void()> &&Func) : MHostTask(std::move(Func)) {}
HostTask(std::function<void(interop_handle)> &&Func) : MInteropTask(std::move(Func)) {}


AllocaCommandBase *getAllocaForReq(Requirement *Req);

EventImplPtr makeEvent(const detail::CG &CG, queue_impl *Queue);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Important: makeEvent should be static

makeEvent is a non-static member function called in ExecCGCommand's constructor member-initializer list as an argument to the Command base-class constructor — meaning it runs before Command is initialized. Inside, it calls ResEvent->setCommand(this) where this points to a partially-constructed object (no base class members have been written yet).

This is technically safe only because makeEvent happens to not access any ExecCGCommand or Command members, but a future change to the function could silently read uninitialized state. Marking it static makes the constraint explicit and prevents accidental access:

Suggested change
EventImplPtr makeEvent(const detail::CG &CG, queue_impl *Queue);
static EventImplPtr makeEvent(const detail::CG &CG, queue_impl *Queue);

The corresponding definition in commands.cpp should also gain the static qualifier (or remove the class scope — either is acceptable for a private static helper).

Comment on lines +314 to 315
case sycl::detail::CGType::NativeHostTask:
case sycl::detail::CGType::None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔵 Suggestion: Document the intentional fallthrough

NativeHostTask falls through silently to None and returns nullptr. This is correct — native host tasks cannot be recorded in a graph node (the exception is thrown earlier in handler::finalize) — but the silent fallthrough may confuse future readers. A brief [[fallthrough]] with a comment (or a separate return nullptr with an explanation) would make the intent clear:

Suggested change
case sycl::detail::CGType::NativeHostTask:
case sycl::detail::CGType::None:
case sycl::detail::CGType::NativeHostTask:
// NativeHostTask is not recordable; graph recording throws before reaching here.
[[fallthrough]];
case sycl::detail::CGType::None:
return nullptr;

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