Skip to content

[SYCL] Fix unreleased events returned by queue USM API #1979

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 1 commit into from
Jun 25, 2020
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
32 changes: 22 additions & 10 deletions sycl/source/detail/queue_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <CL/sycl/detail/memory_manager.hpp>
#include <CL/sycl/detail/pi.hpp>
#include <CL/sycl/device.hpp>
#include <detail/event_impl.hpp>
#include <detail/queue_impl.hpp>

#include <cstring>
Expand Down Expand Up @@ -39,48 +40,59 @@ template <> device queue_impl::get_info<info::queue::device>() const {
return get_device();
}

static event prepareUSMEvent(shared_ptr_class<detail::queue_impl> QueueImpl,
RT::PiEvent NativeEvent) {
auto EventImpl = std::make_shared<detail::event_impl>(QueueImpl);
EventImpl->getHandleRef() = NativeEvent;
EventImpl->setContextImpl(detail::getSyclObjImpl(QueueImpl->get_context()));
return detail::createSyclObjFromImpl<event>(EventImpl);
}

event queue_impl::memset(shared_ptr_class<detail::queue_impl> Impl, void *Ptr,
int Value, size_t Count) {
context Context = get_context();
RT::PiEvent Event = nullptr;
MemoryManager::fill_usm(Ptr, Impl, Count, Value, /*DepEvents*/ {}, Event);
RT::PiEvent NativeEvent = nullptr;
MemoryManager::fill_usm(Ptr, Impl, Count, Value, /*DepEvents*/ {},
NativeEvent);

if (Context.is_host())
return event();

event ResEvent{pi::cast<cl_event>(Event), Context};
event ResEvent = prepareUSMEvent(Impl, NativeEvent);
addUSMEvent(ResEvent);
return ResEvent;
}

event queue_impl::memcpy(shared_ptr_class<detail::queue_impl> Impl, void *Dest,
const void *Src, size_t Count) {
context Context = get_context();
RT::PiEvent Event = nullptr;
MemoryManager::copy_usm(Src, Impl, Count, Dest, /*DepEvents*/ {}, Event);
RT::PiEvent NativeEvent = nullptr;
MemoryManager::copy_usm(Src, Impl, Count, Dest, /*DepEvents*/ {},
NativeEvent);

if (Context.is_host())
return event();

event ResEvent{pi::cast<cl_event>(Event), Context};
event ResEvent = prepareUSMEvent(Impl, NativeEvent);
addUSMEvent(ResEvent);
return ResEvent;
}

event queue_impl::mem_advise(const void *Ptr, size_t Length,
event queue_impl::mem_advise(shared_ptr_class<detail::queue_impl> Impl,
const void *Ptr, size_t Length,
pi_mem_advice Advice) {
context Context = get_context();
if (Context.is_host()) {
return event();
}

// non-Host device
RT::PiEvent Event = nullptr;
RT::PiEvent NativeEvent = nullptr;
const detail::plugin &Plugin = getPlugin();
Plugin.call<PiApiKind::piextUSMEnqueueMemAdvise>(getHandleRef(), Ptr, Length,
Advice, &Event);
Advice, &NativeEvent);

event ResEvent{pi::cast<cl_event>(Event), Context};
event ResEvent = prepareUSMEvent(Impl, NativeEvent);
addUSMEvent(ResEvent);
return ResEvent;
}
Expand Down
4 changes: 3 additions & 1 deletion sycl/source/detail/queue_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,12 @@ class queue_impl {
/// Provides additional information to the underlying runtime about how
/// different allocations are used.
///
/// \param Impl is a shared_ptr to this queue.
/// \param Ptr is a USM pointer to the allocation.
/// \param Length is a number of bytes in the allocation.
/// \param Advice is a device-defined advice for the specified allocation.
event mem_advise(const void *Ptr, size_t Length, pi_mem_advice Advice);
event mem_advise(shared_ptr_class<queue_impl> Impl, const void *Ptr,
size_t Length, pi_mem_advice Advice);

/// Puts exception to the list of asynchronous ecxeptions.
///
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/queue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ event queue::memcpy(void *dest, const void *src, size_t count) {
}

event queue::mem_advise(const void *ptr, size_t length, pi_mem_advice advice) {
return impl->mem_advise(ptr, length, advice);
return impl->mem_advise(impl, ptr, length, advice);
}

event queue::submit_impl(function_class<void(handler &)> CGH,
Expand Down
13 changes: 11 additions & 2 deletions sycl/unittests/queue/wait.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ struct TestCtx {

context &Ctx;
int NEventsWaitedFor = 0;
int EventReferenceCount = 0;
};

std::unique_ptr<TestCtx> TestContext;
Expand All @@ -29,6 +30,7 @@ pi_result redefinedUSMEnqueueMemset(pi_queue queue, void *ptr, pi_int32 value,
pi_event *event) {
// Provide a dummy non-nullptr value
*event = reinterpret_cast<pi_event>(1);
TestContext->EventReferenceCount = 1;
return PI_SUCCESS;
}

Expand All @@ -50,9 +52,15 @@ pi_result redefinedEventGetInfo(pi_event event, pi_event_info param_name,
return PI_SUCCESS;
}

pi_result redefinedEventRetain(pi_event event) { return PI_SUCCESS; }
pi_result redefinedEventRetain(pi_event event) {
++TestContext->EventReferenceCount;
return PI_SUCCESS;
}

pi_result redefinedEventRelease(pi_event event) { return PI_SUCCESS; }
pi_result redefinedEventRelease(pi_event event) {
--TestContext->EventReferenceCount;
return PI_SUCCESS;
}

// Check that the USM events are cleared from the queue upon call to wait(),
// so that they are not waited for multiple times.
Expand Down Expand Up @@ -87,6 +95,7 @@ TEST(QueueWaitTest, USMEventClear) {
Q.memset(HostAlloc, 42, 1);
Q.wait();
ASSERT_EQ(TestContext->NEventsWaitedFor, 1);
ASSERT_EQ(TestContext->EventReferenceCount, 0);
Q.wait();
ASSERT_EQ(TestContext->NEventsWaitedFor, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

The event reference count should be nil after the second call to wait() also, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I just don't think it's worth checking once it already hits 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway it would be nice to have it stressed in the test.

}