Skip to content

[SYCL][CUDA] Use cuEventQuery to check event completion status #3544

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
Apr 16, 2021
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
25 changes: 21 additions & 4 deletions sycl/plugins/cuda/pi_cuda.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -322,9 +322,9 @@ pi_result cuda_piEventRetain(pi_event event);
/// \endcond

_pi_event::_pi_event(pi_command_type type, pi_context context, pi_queue queue)
: commandType_{type}, refCount_{1}, isCompleted_{false}, isRecorded_{false},
isStarted_{false}, evEnd_{nullptr}, evStart_{nullptr}, evQueued_{nullptr},
queue_{queue}, context_{context} {
: commandType_{type}, refCount_{1}, hasBeenWaitedOn_{false},
isRecorded_{false}, isStarted_{false}, evEnd_{nullptr}, evStart_{nullptr},
evQueued_{nullptr}, queue_{queue}, context_{context} {

assert(type != PI_COMMAND_TYPE_USER);

Expand Down Expand Up @@ -369,6 +369,23 @@ pi_result _pi_event::start() {
return result;
}

bool _pi_event::is_completed() const noexcept {
if (!isRecorded_) {
return false;
}
if (!hasBeenWaitedOn_) {
const CUresult ret = cuEventQuery(evEnd_);
if (ret != CUDA_SUCCESS && ret != CUDA_ERROR_NOT_READY) {
PI_CHECK_ERROR(ret);
return false;
}
if (ret == CUDA_ERROR_NOT_READY) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we can safely set isCompleted_ to true here. Will allow it to take a slightly faster path in case this check is made again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I've changed both _pi_event::is_completed and _pi_event::get_execution_status to non-const.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, now I get the comment you made in the summary. It should be fine as non-const, but if it goes against your better judgement I am okay with you reverting it and keeping is_completed as const. I will leave that up to you.

Either way, LGTM! 😄 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to go on a limb and say once the status is complete , users aren't likely to query it again, so optimizing this shouldn't be necessary. Reverted ;-).

Copy link
Contributor

Choose a reason for hiding this comment

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

This would leave the "_pi_event" object in a malformed state where the isCompleted_ is false for a completed event. Light +1 for making this non-const.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to rename it to hasBeenWaitedOn_ or the like. To my knowledge, isCompleted_ is only used for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaslov-intel You are right, that is no good either. I'm still feeing a bit iffy about making an is_* function non-const though.

I like the idea of hasBeenWaitedOn_ as well. If there are no objections, I will move forward with this in a couple of hours!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}
return true;
}

pi_uint64 _pi_event::get_queued_time() const {
float miliSeconds = 0.0f;
assert(is_started());
Expand Down Expand Up @@ -430,7 +447,7 @@ pi_result _pi_event::wait() {
pi_result retErr;
try {
retErr = PI_CHECK_ERROR(cuEventSynchronize(evEnd_));
isCompleted_ = true;
hasBeenWaitedOn_ = true;
} catch (pi_result error) {
retErr = error;
}
Expand Down
7 changes: 4 additions & 3 deletions sycl/plugins/cuda/pi_cuda.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ class _pi_event {

bool is_started() const noexcept { return isStarted_; }

bool is_completed() const noexcept { return isCompleted_; };
bool is_completed() const noexcept;

pi_int32 get_execution_status() const noexcept {

Expand Down Expand Up @@ -462,8 +462,9 @@ class _pi_event {

std::atomic_uint32_t refCount_; // Event reference count.

bool isCompleted_; // Signifies whether the operations have completed
//
bool hasBeenWaitedOn_; // Signifies whether the event has been waited
// on through a call to wait(), which implies
// that it has completed.

bool isRecorded_; // Signifies wether a native CUDA event has been recorded
// yet.
Expand Down