Skip to content

[NFCI][SYCL] Copy handler::MQueue to handler_impl #18830

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 2 commits into from
Jun 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions sycl/source/accessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,6 @@
namespace sycl {
inline namespace _V1 {
namespace detail {
device getDeviceFromHandler(handler &cgh) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exported, I've moved it to handler.cpp.

assert((cgh.MQueue || getSyclObjImpl(cgh)->MGraph) &&
"One of MQueue or MGraph should be nonnull!");
if (cgh.MQueue)
return cgh.MQueue->get_device();

return getSyclObjImpl(cgh)->MGraph->getDevice();
}

// property::no_init is supported now for
// accessor
// host_accessor
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/context_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ inline namespace _V1 {
// Forward declaration
class device;
namespace detail {
class context_impl : std::enable_shared_from_this<context_impl> {
class context_impl : public std::enable_shared_from_this<context_impl> {
struct private_tag {
explicit private_tag() = default;
};
Expand Down
4 changes: 2 additions & 2 deletions sycl/source/detail/graph_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ graph_impl::add(std::function<void(handler &)> CGF,
std::vector<std::shared_ptr<node_impl>> &Deps) {
(void)Args;
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
detail::handler_impl HandlerImpl{shared_from_this()};
detail::handler_impl HandlerImpl{*this};
sycl::handler Handler{&HandlerImpl, std::shared_ptr<detail::queue_impl>{}};
#else
sycl::handler Handler{shared_from_this()};
Expand Down Expand Up @@ -2284,7 +2284,7 @@ void dynamic_command_group_impl::finalizeCGFList(
// Handler defined inside the loop so it doesn't appear to the runtime
// as a single command-group with multiple commands inside.
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
detail::handler_impl HandlerImpl{MGraph};
detail::handler_impl HandlerImpl{*MGraph};
sycl::handler Handler{&HandlerImpl, std::shared_ptr<detail::queue_impl>{}};
#else
sycl::handler Handler{MGraph};
Expand Down
55 changes: 46 additions & 9 deletions sycl/source/detail/handler_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "sycl/handler.hpp"
#include <detail/cg.hpp>
#include <detail/graph_impl.hpp>
#include <detail/kernel_bundle_impl.hpp>
#include <memory>
#include <sycl/ext/oneapi/experimental/graph.hpp>
Expand All @@ -31,15 +32,13 @@ enum class HandlerSubmissionState : std::uint8_t {

class handler_impl {
public:
handler_impl(queue_impl *SubmissionSecondaryQueue, bool EventNeeded)
handler_impl(queue_impl &Queue, queue_impl *SubmissionSecondaryQueue,
bool EventNeeded)
: MSubmissionSecondaryQueue(SubmissionSecondaryQueue),
MEventNeeded(EventNeeded) {};
MEventNeeded(EventNeeded), MQueueOrGraph{Queue} {};

handler_impl(
std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> Graph)
: MGraph{Graph} {}

handler_impl() = default;
handler_impl(ext::oneapi::experimental::detail::graph_impl &Graph)
: MQueueOrGraph{Graph} {}
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have any %if preview-breaking-changes-supported Graph E2E tests to catch this. When we call this constructor with under __INTEL_PREVIEW_BREAKING_CHANGES
https://github.com/intel/llvm/blob/sycl/sycl/source/detail/graph_impl.cpp#L507 it will pass a std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> from shared_from_this() can that compile after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

source/detail is compiled unconditionally in two modes, because we provide two variants of libsycl.so/libsycl-preview.so. All existing E2E tests are run in preview mode in this nightly task as well:

- name: Preview mode on SPR/PVC
runner: '["Linux", "pvc"]'
image_options: -u 1001 --device=/dev/dri -v /dev/dri/by-path:/dev/dri/by-path --privileged --cap-add SYS_ADMIN
target_devices: level_zero:gpu
extra_lit_opts: --param test-preview-mode=True

That said, I'll take another look locally to be 100% sure, but I wouldn't expect any failures here.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, in that case I've approved the PR, but if you could double check locally that would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already modified as part of this PR - see changes in the above file (graph_impl.cpp).


void setStateExplicitKernelBundle() {
if (MSubmissionState == HandlerSubmissionState::SPEC_CONST_SET_STATE)
Expand Down Expand Up @@ -165,8 +164,46 @@ class handler_impl {
/// manipulations with version are required
detail::CGType MCGType = detail::CGType::None;

/// The graph that is associated with this handler.
std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> MGraph;
// This handler is associated with either a queue or a graph.
using graph_impl = ext::oneapi::experimental::detail::graph_impl;
const std::variant<std::reference_wrapper<queue_impl>,
std::reference_wrapper<graph_impl>>
MQueueOrGraph;

queue_impl *get_queue_or_null() {
auto *Queue =
std::get_if<std::reference_wrapper<queue_impl>>(&MQueueOrGraph);
return Queue ? &Queue->get() : nullptr;
}
queue_impl &get_queue() {
return std::get<std::reference_wrapper<queue_impl>>(MQueueOrGraph).get();
}
graph_impl *get_graph_or_null() {
auto *Graph =
std::get_if<std::reference_wrapper<graph_impl>>(&MQueueOrGraph);
return Graph ? &Graph->get() : nullptr;
}
graph_impl &get_graph() {
return std::get<std::reference_wrapper<graph_impl>>(MQueueOrGraph).get();
}

// Make the following methods templates to avoid circular dependencies for the
// includes.
template <typename Self = handler_impl> detail::device_impl &get_device() {
Self *self = this;
if (auto *Queue = self->get_queue_or_null())
return Queue->getDeviceImpl();
else
return self->get_graph().getDeviceImpl();
}
template <typename Self = handler_impl> context_impl &get_context() {
Self *self = this;
if (auto *Queue = self->get_queue_or_null())
return *Queue->getContextImplPtr();
else
return *self->get_graph().getContextImplPtr();
}

/// If we are submitting a graph using ext_oneapi_graph this will be the graph
/// to be executed.
std::shared_ptr<ext::oneapi::experimental::detail::exec_graph_impl>
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/queue_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ queue_impl::submit_impl(const detail::type_erased_cgfo_ty &CGF,
const detail::code_location &Loc, bool IsTopCodeLoc,
const v1::SubmissionInfo &SubmitInfo) {
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
detail::handler_impl HandlerImplVal(SecondaryQueue, CallerNeedsEvent);
detail::handler_impl HandlerImplVal(*this, SecondaryQueue, CallerNeedsEvent);
detail::handler_impl *HandlerImpl = &HandlerImplVal;
// Inlining `Self` results in a crash when SYCL RT is built using MSVC with
// optimizations enabled. No crash if built using OneAPI.
Expand Down
Loading