[SYCL] Improve sycl::event::get_profiling_info exception message if default constructed #6476
Conversation
[SYCL][ABI-break] Remove kernel::get_work_group_info (intel#6414)
The default constructed sycl::event would emit a misleading message when querying profiling information.The message would assume a queue is attached to such sycl::event, even if it's not the case. Added a condition to handle such case, and emit a more proper message. Signed-off-by: Rauf, Rana <[email protected]>
There was a problem hiding this comment.
The additional field is added solely for the purpose of slightly more clear exception message....
I do not object, but also do not support the idea.
Would be useful to hear from @bader and @steffenlarsen
There was a problem hiding this comment.
The clearer error message is definitely a great addition, but I think it can be done without the new member. Sadly we cannot base it on whether the weak_ptr has expired, as we could have an event for which the associated queue had profiling info but has since been destroyed. However, std::weak_ptr::owner_before specifies that
The order is such that two smart pointers compare equivalent only if they are both empty or if they both own the same object ...
Using this, we can determine if there was a (potentially now expired) queue associated with event. See https://godbolt.org/z/3v8144j1o for an example of this. Of the top of my head, the only potential problem I could see here would be if at some point reset is called on the weak_ptr, but I don't see why we would ever need that.
On a different note, could you please add a test-case to GetProfilingInfo for the new exception message?
bader
changed the title
Signed-off-by: Rauf, Rana <[email protected]>
Signed-off-by: Rauf, Rana <[email protected]>
The default constructed sycl::event would emit a misleading message when querying profiling information.The message would assume a queue is attached to such sycl::event, even if it's not the case. Added a condition to handle such case, and emit a more proper message. Signed-off-by: Rauf, Rana <[email protected]>
Signed-off-by: Rauf, Rana <[email protected]>
Signed-off-by: Rauf, Rana <[email protected]>
… event_exception
|
\verify with intel/llvm-test-suite#1120 |
|
I think it should be forward slash i.e. /verify, not \verify. |
|
Thanks for pointing that out |
|
/verify with intel/llvm-test-suite#1120 |
|
/verify with intel/llvm-test-suite#1120 |
The default constructed sycl::event would emit a misleading message when querying profiling information.The message would assume a queue is attached to such sycl::event, even if it's not the case. Added a condition to handle such case, and emit a more proper message. Signed-off-by: Rauf, Rana <[email protected]>
Signed-off-by: Rauf, Rana <[email protected]>
Signed-off-by: Rauf, Rana <[email protected]>
The default constructed sycl::event would emit a misleading message when querying profiling information.The message would assume a queue is attached to such sycl::event, even if it's not the case. Added a condition to handle such case, and emit a more proper message. Signed-off-by: Rauf, Rana <[email protected]>
Signed-off-by: Rauf, Rana <[email protected]>
… event_exception
|
@intel/llvm-gatekeepers ping to merge commit |
The default constructed sycl::event would emit a misleading message when querying profiling information. The message would assume a queue is attached to such sycl::event, even if it's not the case. Added a condition to handle such case, and emit a more proper message, and updated appropriate unit test.