Skip to content

[SYCL] Add error handling for sycl::event::get_profiling_info() #5623

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

Jump to bottom
Merged
merged 16 commits into from
Mar 5, 2022
Merged

[SYCL] Add error handling for sycl::event::get_profiling_info() #5623

merged 16 commits into from
Mar 5, 2022

Conversation

HabKaffee
Copy link
Contributor

This patch implements a throwing of sycl::exception if sycl::event::get_profiling_info() called when sycl::queue does not have a property::queue::enable_profiling in its sycl::property_list.
This PR fixes #5574

@HabKaffee HabKaffee requested a review from a team as a code owner February 21, 2022 10:02
steffenlarsen
steffenlarsen reviewed Feb 21, 2022
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned in the comment, I think it would be safer to check early and use that during checks. Otherwise we could have queue disappear before we do profiling, which would allow profiling calls even if the queue didn't have it enabled.

steffenlarsen
steffenlarsen reviewed Feb 22, 2022
View reviewed changes
steffenlarsen
steffenlarsen reviewed Feb 22, 2022
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Just two more suggestions, then I think I am happy with it!

steffenlarsen
steffenlarsen previously approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Good stuff! Thanks for addressing my concerns. LGTM.

@HabKaffee
Copy link
Contributor Author

I'll investigate what is wrong with llvm-test-suite/Plugin/level_zero_queue_profiling.cpp

@HabKaffee HabKaffee mentioned this pull request Feb 22, 2022
Merged
@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

llvm-test-suite/Plugin/level_zero_queue_profiling.cpp changed. PR in intel/llvm-test-suite is approved. Changes passed verification.

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

@steffenlarsen could you please approve PR? Nothing except clang format has changed.

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

steffenlarsen
steffenlarsen previously approved these changes Mar 2, 2022
View reviewed changes
@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

@steffenlarsen Please, take a look on that fix. I added skips for HIP and CUDA backends in my unittests.

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

/verify with intel/llvm-test-suite#863

@HabKaffee
Copy link
Contributor Author

Some tests, which are failing, was fixed in related PR to intel/llvm-test-suite link to PR

@bader bader changed the title [SYCL] Add throwing an exception if sycl::event::get_profiling_info() called without needed sycl::queue property [SYCL] Add error handling for sycl::event::get_profiling_info() Mar 5, 2022
@bader bader merged commit ec74a5c into intel:sycl Mar 5, 2022
bader pushed a commit to intel/llvm-test-suite that referenced this pull request Mar 5, 2022
@HabKaffee
[SYCL] Change level_zero_queue_profiling.cpp (#863)
c5012d6 
This patch change test level_zero_queue_profiling.cpp because of changes, implemented by intel/llvm#5623
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@HabKaffee
[SYCL] Change level_zero_queue_profiling.cpp (intel/llvm-test-suite#863)
b497bd9 
This patch change test level_zero_queue_profiling.cpp because of changes, implemented by intel#5623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@s-kanaev s-kanaev Awaiting requested review from s-kanaev s-kanaev is a code owner automatically assigned from intel/llvm-reviewers-runtime

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

@alexbatashev alexbatashev Awaiting requested review from alexbatashev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SYCL] get_profiling_info doesnt throw an exception
3 participants
@HabKaffee @steffenlarsen @bader