Skip to content

[SYCL] Use raw pointers in enqueue flow #18306

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.

Sign up for GitHub

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
aelovikov-intel merged 2 commits into from
May 6, 2025
Merged

[SYCL] Use raw pointers in enqueue flow #18306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47f9aa2
Use raw pointers to event_impl in enqueue flow
May 5, 2025
58afdde
Use raw pointer to kernel_impl in enqueueImpKernel
May 5, 2025
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
4 changes: 2 additions & 2 deletions sycl/source/detail/kernel_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ class kernel_impl {
ur_program_handle_t getProgramRef() const { return MProgram; }
ContextImplPtr getContextImplPtr() const { return MContext; }

std::mutex &getNoncacheableEnqueueMutex() {
std::mutex &getNoncacheableEnqueueMutex() const {
return MNoncacheableEnqueueMutex;
}

Expand All @@ -247,7 +247,7 @@ class kernel_impl {
const DeviceImageImplPtr MDeviceImageImpl;
const KernelBundleImplPtr MKernelBundleImpl;
bool MIsInterop = false;
std::mutex MNoncacheableEnqueueMutex;
mutable std::mutex MNoncacheableEnqueueMutex;
Copy link
Contributor

@aelovikov-intel aelovikov-intel May 6, 2025

Choose a reason for hiding this comment

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

I'm not sure what's our approach for const, we probably didn't care before because shared pointers could always give us non-const ptr even if they themselves were const.

@sergey-semenov , any preferences? I think the pointer we get below is non-const, so we can avoid this bit by simply changing the signature to accept non-const.

Copy link
Contributor Author

@vinser52 vinser52 May 6, 2025

Choose a reason for hiding this comment

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

My preference is that if the object itself is not modified, then a const pointer should be used and use mutable std::mutex since it might be required for read-only concurrent access synchronization.

Copy link
Contributor

@aelovikov-intel aelovikov-intel May 6, 2025

Choose a reason for hiding this comment

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

My issue is that we (most likely) don't do that consistently. I'd rather have "wrong" behavior everywhere than the "right" one somewhere only. But this isn't big deal for me.

Copy link
Contributor

@sergey-semenov sergey-semenov May 6, 2025

Choose a reason for hiding this comment

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

we probably didn't care before because shared pointers could always give us non-const ptr even if they themselves were const.

That's probably true in some (most?) places. I don't have a strong opinion on this, but without knowing exactly how widespread the problem is, I'd rather not contribute to it and start fixing the pieces that we hit, so I'm leaning to sticking with mutable here.

const KernelArgMask *MKernelArgMaskPtr;
std::mutex *MCacheMutex = nullptr;
mutable std::string MName;
Expand Down
18 changes: 8 additions & 10 deletions sycl/source/detail/scheduler/commands.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2380,8 +2380,7 @@ static ur_result_t SetKernelParamsAndLaunch(
const QueueImplPtr &Queue, std::vector<ArgDesc> &Args,
const std::shared_ptr<device_image_impl> &DeviceImageImpl,
ur_kernel_handle_t Kernel, NDRDescT &NDRDesc,
std::vector<ur_event_handle_t> &RawEvents,
const detail::EventImplPtr &OutEventImpl,
std::vector<ur_event_handle_t> &RawEvents, detail::event_impl *OutEventImpl,
const KernelArgMask *EliminatedArgMask,
const std::function<void *(Requirement *Req)> &getMemAllocationFunc,
bool IsCooperative, bool KernelUsesClusterLaunch,
Expand Down Expand Up @@ -2651,9 +2650,8 @@ ur_result_t enqueueImpCommandBufferKernel(
void enqueueImpKernel(
const QueueImplPtr &Queue, NDRDescT &NDRDesc, std::vector<ArgDesc> &Args,
const std::shared_ptr<detail::kernel_bundle_impl> &KernelBundleImplPtr,
const std::shared_ptr<detail::kernel_impl> &MSyclKernel,
KernelNameStrRefT KernelName, std::vector<ur_event_handle_t> &RawEvents,
const detail::EventImplPtr &OutEventImpl,
const detail::kernel_impl *MSyclKernel, KernelNameStrRefT KernelName,
std::vector<ur_event_handle_t> &RawEvents, detail::event_impl *OutEventImpl,
const std::function<void *(Requirement *Req)> &getMemAllocationFunc,
ur_kernel_cache_config_t KernelCacheConfig, const bool KernelIsCooperative,
const bool KernelUsesClusterLaunch, const size_t WorkGroupMemorySize,
Expand Down Expand Up @@ -2761,7 +2759,7 @@ ur_result_t enqueueReadWriteHostPipe(const QueueImplPtr &Queue,
const std::string &PipeName, bool blocking,
void *ptr, size_t size,
std::vector<ur_event_handle_t> &RawEvents,
const detail::EventImplPtr &OutEventImpl,
detail::event_impl *OutEventImpl,
Copy link
Contributor

@aelovikov-intel aelovikov-intel May 6, 2025

Choose a reason for hiding this comment

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

I can somewhat believe that in-parameters don't change ownership model, but modifying out-parameter and saying "it's unchanged, trust me" sounds very suspicious. Can you explain more?

Or maybe in other words, is that out-parameter even necessary?

Copy link
Contributor Author

@vinser52 vinser52 May 6, 2025

Choose a reason for hiding this comment

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

If OutEventImpl is not nullptr then this function calls OutEventImpl->setHandle(UREvent); on L2805.

The same is true for the SetKernelParamsAndLaunch function.

Copy link
Contributor

@aelovikov-intel aelovikov-intel May 6, 2025

Choose a reason for hiding this comment

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

I've updated PR description summarizing my understanding. Can you please check if that's correct and you agree with the change?

Copy link
Contributor Author

@vinser52 vinser52 May 6, 2025

Choose a reason for hiding this comment

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

Looks good. Thank you!
I just fixed one misprint: "cand the modified interfaces update" -> "and the modified interfaces updat"

Copy link
Contributor

@aelovikov-intel aelovikov-intel May 6, 2025

Choose a reason for hiding this comment

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

Yeah, sorry to be a pain, but mistakes can result in leaks/use-after-free and other flaky failures, so I want to be extra careful with this stuff.

Once Sergey replies about const this is LGTM.

bool read) {
assert(Queue &&
"ReadWrite host pipe submissions should have an associated queue");
Expand Down Expand Up @@ -3122,7 +3120,7 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {

ur_event_handle_t UREvent = nullptr;
ur_event_handle_t *Event = DiscardUrEvent ? nullptr : &UREvent;
detail::EventImplPtr EventImpl = DiscardUrEvent ? nullptr : MEvent;
detail::event_impl *EventImpl = DiscardUrEvent ? nullptr : MEvent.get();

auto SetEventHandleOrDiscard = [&]() {
if (Event)
Expand Down Expand Up @@ -3237,7 +3235,7 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
(!SyclKernel || SyclKernel->hasSYCLMetadata()) &&
ProgramManager::getInstance().kernelUsesAssert(KernelName);
if (KernelUsesAssert) {
EventImpl = MEvent;
EventImpl = MEvent.get();
}
}

Expand All @@ -3248,7 +3246,7 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
assert(BinImage && "Failed to obtain a binary image.");
}
enqueueImpKernel(MQueue, NDRDesc, Args, ExecKernel->getKernelBundle(),
SyclKernel, KernelName, RawEvents, EventImpl,
SyclKernel.get(), KernelName, RawEvents, EventImpl,
getMemAllocationFunc, ExecKernel->MKernelCacheConfig,
ExecKernel->MKernelIsCooperative,
ExecKernel->MKernelUsesClusterLaunch,
Expand Down Expand Up @@ -3645,7 +3643,7 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
bool read = ExecReadWriteHostPipe->isReadHostPipe();

if (!EventImpl) {
EventImpl = MEvent;
EventImpl = MEvent.get();
}
return enqueueReadWriteHostPipe(MQueue, pipeName, blocking, hostPtr,
typeSize, RawEvents, EventImpl, read);
Expand Down
7 changes: 3 additions & 4 deletions sycl/source/detail/scheduler/commands.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,15 +614,14 @@ ur_result_t enqueueReadWriteHostPipe(const QueueImplPtr &Queue,
const std::string &PipeName, bool blocking,
void *ptr, size_t size,
std::vector<ur_event_handle_t> &RawEvents,
const detail::EventImplPtr &OutEventImpl,
detail::event_impl *OutEventImpl,
bool read);

void enqueueImpKernel(
const QueueImplPtr &Queue, NDRDescT &NDRDesc, std::vector<ArgDesc> &Args,
const std::shared_ptr<detail::kernel_bundle_impl> &KernelBundleImplPtr,
const std::shared_ptr<detail::kernel_impl> &MSyclKernel,
KernelNameStrRefT KernelName, std::vector<ur_event_handle_t> &RawEvents,
const detail::EventImplPtr &Event,
const detail::kernel_impl *MSyclKernel, KernelNameStrRefT KernelName,
std::vector<ur_event_handle_t> &RawEvents, detail::event_impl *OutEventImpl,
const std::function<void *(Requirement *Req)> &getMemAllocationFunc,
ur_kernel_cache_config_t KernelCacheConfig, bool KernelIsCooperative,
const bool KernelUsesClusterLaunch, const size_t WorkGroupMemorySize,
Expand Down
14 changes: 7 additions & 7 deletions sycl/source/handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,13 +537,13 @@ event handler::finalize() {
detail::retrieveKernelBinary(MQueue, MKernelName.data());
assert(BinImage && "Failed to obtain a binary image.");
}
enqueueImpKernel(
MQueue, impl->MNDRDesc, impl->MArgs, KernelBundleImpPtr, MKernel,
MKernelName.data(), RawEvents,
DiscardEvent ? detail::EventImplPtr{} : LastEventImpl, nullptr,
impl->MKernelCacheConfig, impl->MKernelIsCooperative,
impl->MKernelUsesClusterLaunch, impl->MKernelWorkGroupMemorySize,
BinImage);
enqueueImpKernel(MQueue, impl->MNDRDesc, impl->MArgs,
KernelBundleImpPtr, MKernel.get(), MKernelName.data(),
RawEvents,
DiscardEvent ? nullptr : LastEventImpl.get(), nullptr,
impl->MKernelCacheConfig, impl->MKernelIsCooperative,
impl->MKernelUsesClusterLaunch,
impl->MKernelWorkGroupMemorySize, BinImage);
#ifdef XPTI_ENABLE_INSTRUMENTATION
// Emit signal only when event is created
if (!DiscardEvent) {
Expand Down
Loading