Skip to content

Commit 5281e90

Browse files
[NFCI][SYCL] Copy handler::MQueue to handler_impl
Ideally, it should be just moved there, but doing the whole move in one PR would be harder to review as it would require multiple internal API changes to accept `queue_impl` by raw ptr/ref instead of `std::shared_ptr<queue_impl>` value/ref. As such, this PR does the following: * Store `std::variant` of queue/graph instead of just graph as two are mutually exclusive * Store them by reference (through `std::reference_wrapper` as 'std::variant' can't store raw references directly) instead of `std::shared_ptr<graph_impl>`. The reason for that is that the object `handler` is submitted to is guaranteed to be alive for the whole lifetime of the `handler` and we don't need to extend the lifetime of queue/graph. Corresponding changes for `handler::MQueue` have been implemented recently already (although via `std::shared_ptr<queue_impl> &` and not raw reference) but are limited to prevew-only. Do the same for graphs here, essentially. * Update all uses of the old `handler_impl::MGraph` data member as it needs to go through new getters accessing the `std::variant` described above. * Update some of the direct usages of `handler::MQueue` that don't require APIs refactoring elsewhere. The remaining uses are left to the subsequent PR(s). * We'll probably need to keep the `handler::MQueue` initialized properly even after the move is complete and all internal SYCL RT accesses are through `handler_impl` as some direct `handler::MQueue` accesses might have been inlined into the users' applications (I'd be especially worried about reductions).
1 parent 8c528be commit 5281e90

File tree

6 files changed

+125
-88
lines changed

6 files changed

+125
-88
lines changed

sycl/source/accessor.cpp

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,6 @@
1414
namespace sycl {
1515
inline namespace _V1 {
1616
namespace detail {
17-
device getDeviceFromHandler(handler &cgh) {
18-
assert((cgh.MQueue || getSyclObjImpl(cgh)->MGraph) &&
19-
"One of MQueue or MGraph should be nonnull!");
20-
if (cgh.MQueue)
21-
return cgh.MQueue->get_device();
22-
23-
return getSyclObjImpl(cgh)->MGraph->getDevice();
24-
}
25-
2617
// property::no_init is supported now for
2718
// accessor
2819
// host_accessor

sycl/source/detail/context_impl.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ inline namespace _V1 {
2929
// Forward declaration
3030
class device;
3131
namespace detail {
32-
class context_impl : std::enable_shared_from_this<context_impl> {
32+
class context_impl : public std::enable_shared_from_this<context_impl> {
3333
struct private_tag {
3434
explicit private_tag() = default;
3535
};

sycl/source/detail/graph_impl.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ graph_impl::add(std::function<void(handler &)> CGF,
504504
std::vector<std::shared_ptr<node_impl>> &Deps) {
505505
(void)Args;
506506
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
507-
detail::handler_impl HandlerImpl{shared_from_this()};
507+
detail::handler_impl HandlerImpl{*this};
508508
sycl::handler Handler{&HandlerImpl, std::shared_ptr<detail::queue_impl>{}};
509509
#else
510510
sycl::handler Handler{shared_from_this()};
@@ -2284,7 +2284,7 @@ void dynamic_command_group_impl::finalizeCGFList(
22842284
// Handler defined inside the loop so it doesn't appear to the runtime
22852285
// as a single command-group with multiple commands inside.
22862286
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
2287-
detail::handler_impl HandlerImpl{MGraph};
2287+
detail::handler_impl HandlerImpl{*MGraph};
22882288
sycl::handler Handler{&HandlerImpl, std::shared_ptr<detail::queue_impl>{}};
22892289
#else
22902290
sycl::handler Handler{MGraph};

sycl/source/detail/handler_impl.hpp

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

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

3233
class handler_impl {
3334
public:
34-
handler_impl(queue_impl *SubmissionSecondaryQueue, bool EventNeeded)
35+
handler_impl(queue_impl &Queue, queue_impl *SubmissionSecondaryQueue,
36+
bool EventNeeded)
3537
: MSubmissionSecondaryQueue(SubmissionSecondaryQueue),
36-
MEventNeeded(EventNeeded) {};
38+
MEventNeeded(EventNeeded), MQueueOrGraph{Queue} {};
3739

38-
handler_impl(
39-
std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> Graph)
40-
: MGraph{Graph} {}
41-
42-
handler_impl() = default;
40+
handler_impl(ext::oneapi::experimental::detail::graph_impl &Graph)
41+
: MQueueOrGraph{Graph} {}
4342

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

168-
/// The graph that is associated with this handler.
169-
std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> MGraph;
167+
// This handler is associated with either a queue or a graph.
168+
using graph_impl = ext::oneapi::experimental::detail::graph_impl;
169+
const std::variant<std::reference_wrapper<queue_impl>,
170+
std::reference_wrapper<graph_impl>>
171+
MQueueOrGraph;
172+
173+
queue_impl *get_queue_or_null() {
174+
auto *Queue =
175+
std::get_if<std::reference_wrapper<queue_impl>>(&MQueueOrGraph);
176+
return Queue ? &Queue->get() : nullptr;
177+
}
178+
queue_impl &get_queue() {
179+
return std::get<std::reference_wrapper<queue_impl>>(MQueueOrGraph).get();
180+
}
181+
graph_impl *get_graph_or_null() {
182+
auto *Graph =
183+
std::get_if<std::reference_wrapper<graph_impl>>(&MQueueOrGraph);
184+
return Graph ? &Graph->get() : nullptr;
185+
}
186+
graph_impl &get_graph() {
187+
return std::get<std::reference_wrapper<graph_impl>>(MQueueOrGraph).get();
188+
}
189+
190+
// Make the following methods templates to avoid circular dependencies for the
191+
// includes.
192+
template <typename Self = handler_impl> detail::device_impl &get_device() {
193+
Self *self = this;
194+
if (auto *queue = self->get_queue_or_null())
195+
return queue->getDeviceImpl();
196+
else
197+
return self->get_graph().getDeviceImpl();
198+
}
199+
template <typename Self = handler_impl> context_impl &get_context() {
200+
Self *self = this;
201+
if (auto *queue = self->get_queue_or_null())
202+
return *queue->getContextImplPtr();
203+
else
204+
return *self->get_graph().getContextImplPtr();
205+
}
206+
170207
/// If we are submitting a graph using ext_oneapi_graph this will be the graph
171208
/// to be executed.
172209
std::shared_ptr<ext::oneapi::experimental::detail::exec_graph_impl>

sycl/source/detail/queue_impl.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ queue_impl::submit_impl(const detail::type_erased_cgfo_ty &CGF,
310310
const detail::code_location &Loc, bool IsTopCodeLoc,
311311
const v1::SubmissionInfo &SubmitInfo) {
312312
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
313-
detail::handler_impl HandlerImplVal(SecondaryQueue, CallerNeedsEvent);
313+
detail::handler_impl HandlerImplVal(*this, SecondaryQueue, CallerNeedsEvent);
314314
detail::handler_impl *HandlerImpl = &HandlerImplVal;
315315
// Inlining `Self` results in a crash when SYCL RT is built using MSVC with
316316
// optimizations enabled. No crash if built using OneAPI.

0 commit comments

Comments
 (0)