-
Notifications
You must be signed in to change notification settings - Fork 788
[SYCL] Clear event_impl dependencies with graph cleanup #4793
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
Changes from all commits
b46adfe
bdb43b8
1ccf2b1
1baab21
a5153d6
00bef9e
a9dfd7c
941b469
ee3502f
b918571
2f5b026
d04461f
dfcba47
3e3803a
b56a925
2727036
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,18 +210,24 @@ void event_impl::wait( | |
|
||
void event_impl::wait_and_throw( | ||
std::shared_ptr<cl::sycl::detail::event_impl> Self) { | ||
Command *Cmd = static_cast<Command *>(Self->getCommand()); | ||
QueueImplPtr submittedQueue = nullptr; | ||
if (Cmd) | ||
submittedQueue = Cmd->getSubmittedQueue(); | ||
Scheduler &Sched = Scheduler::getInstance(); | ||
|
||
QueueImplPtr submittedQueue = nullptr; | ||
{ | ||
Scheduler::ReadLockT Lock(Sched.MGraphLock); | ||
Command *Cmd = static_cast<Command *>(Self->getCommand()); | ||
if (Cmd) | ||
submittedQueue = Cmd->getSubmittedQueue(); | ||
} | ||
wait(Self); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed it |
||
|
||
for (auto &EventImpl : | ||
detail::Scheduler::getInstance().getWaitList(std::move(Self))) { | ||
Command *Cmd = (Command *)EventImpl->getCommand(); | ||
if (Cmd) | ||
Cmd->getSubmittedQueue()->throw_asynchronous(); | ||
{ | ||
Scheduler::ReadLockT Lock(Sched.MGraphLock); | ||
for (auto &EventImpl : getWaitList()) { | ||
Command *Cmd = (Command *)EventImpl->getCommand(); | ||
if (Cmd) | ||
Cmd->getSubmittedQueue()->throw_asynchronous(); | ||
} | ||
} | ||
if (submittedQueue) | ||
submittedQueue->throw_asynchronous(); | ||
|
@@ -325,6 +331,25 @@ pi_native_handle event_impl::getNative() const { | |
return Handle; | ||
} | ||
|
||
std::vector<EventImplPtr> event_impl::getWaitList() { | ||
std::lock_guard<std::mutex> Lock(MMutex); | ||
|
||
std::vector<EventImplPtr> Result; | ||
Result.reserve(MPreparedDepsEvents.size() + MPreparedHostDepsEvents.size()); | ||
Result.insert(Result.end(), MPreparedDepsEvents.begin(), | ||
MPreparedDepsEvents.end()); | ||
Result.insert(Result.end(), MPreparedHostDepsEvents.begin(), | ||
MPreparedHostDepsEvents.end()); | ||
|
||
return Result; | ||
} | ||
|
||
void event_impl::cleanupDependencyEvents() { | ||
std::lock_guard<std::mutex> Lock(MMutex); | ||
MPreparedDepsEvents.clear(); | ||
MPreparedHostDepsEvents.clear(); | ||
} | ||
|
||
} // namespace detail | ||
} // namespace sycl | ||
} // __SYCL_INLINE_NAMESPACE(cl) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ class context_impl; | |
using ContextImplPtr = std::shared_ptr<cl::sycl::detail::context_impl>; | ||
class queue_impl; | ||
using QueueImplPtr = std::shared_ptr<cl::sycl::detail::queue_impl>; | ||
class event_impl; | ||
using EventImplPtr = std::shared_ptr<cl::sycl::detail::event_impl>; | ||
|
||
class event_impl { | ||
public: | ||
|
@@ -175,6 +177,14 @@ class event_impl { | |
return MPreparedHostDepsEvents; | ||
} | ||
|
||
/// Returns vector of event_impl that this event_impl depends on. | ||
/// | ||
/// @return a vector of "immediate" dependencies for this event_impl. | ||
std::vector<EventImplPtr> getWaitList(); | ||
|
||
/// Cleans dependencies of this event_impl | ||
void cleanupDependencyEvents(); | ||
|
||
private: | ||
// When instrumentation is enabled emits trace event for event wait begin and | ||
// returns the telemetry event generated for the wait | ||
|
@@ -192,15 +202,17 @@ class event_impl { | |
void *MCommand = nullptr; | ||
|
||
/// Dependency events prepared for waiting by backend. | ||
std::vector<std::shared_ptr<event_impl>> MPreparedDepsEvents; | ||
std::vector<std::shared_ptr<event_impl>> MPreparedHostDepsEvents; | ||
std::vector<EventImplPtr> MPreparedDepsEvents; | ||
std::vector<EventImplPtr> MPreparedHostDepsEvents; | ||
|
||
enum HostEventState : int { HES_NotComplete = 0, HES_Complete }; | ||
|
||
// State of host event. Employed only for host events and event with no | ||
// backend's representation (e.g. alloca). Used values are listed in | ||
// HostEventState enum. | ||
std::atomic<int> MState; | ||
|
||
std::mutex MMutex; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this mutex needed? Is access to MPreparedDepsEvents and MPreparedHostDepsEvents not guarded by graph R/W lock? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Access to these dependencies is not guarded when the user wants to get them from event::get_wait_list |
||
}; | ||
|
||
} // namespace detail | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,6 +129,7 @@ static void handleVisitedNodes(std::vector<Command *> &Visited) { | |
for (Command *Cmd : Visited) { | ||
if (Cmd->MMarks.MToBeDeleted) { | ||
Cmd->getEvent()->setCommand(nullptr); | ||
Cmd->getEvent()->cleanupDependencyEvents(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to still lead to d-tor to go through DFS and overflowing the stack, isn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Last changes are not lead to stack overflow on the tests that were affected |
||
delete Cmd; | ||
} else | ||
Cmd->MMarks.MVisited = false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this patch, but it seems this function is not thread safe. We accessing
Cmd
here which AFAIK can be removed in graph cleanup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrapped wait_and_throw under mutex