Skip to content
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

[Cuda] Reintroduce catching and reporting of bad_alloc for event object creation #1962

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GeorgeWeb
Copy link
Contributor

Reintroduces the changes from commit c4ae460, which were reverted in related merged commit 1b4a8b8 due to being mistakenly deleted and omitted on rebasing.

This happened because the former changes got split out from PR #1399 to independent changes in PR #1397.

@GeorgeWeb GeorgeWeb requested a review from a team as a code owner August 12, 2024 12:47
@github-actions github-actions bot added the cuda CUDA adapter specific issues label Aug 12, 2024
@GeorgeWeb GeorgeWeb added the ready to merge Added to PR's which are ready to merge label Aug 12, 2024
@igchor
Copy link
Member

igchor commented Aug 12, 2024

Just FYI, handling this exception here is fine but not necessary. We already catch all exceptions in ur_libapi.cpp, e.g. : https://github.com/oneapi-src/unified-runtime/blob/main/source/loader/ur_libapi.cpp#L4831 and that includes handling bad_alloc: https://github.com/oneapi-src/unified-runtime/blob/main/source/common/ur_util.hpp#L280

@GeorgeWeb
Copy link
Contributor Author

GeorgeWeb commented Aug 13, 2024

We already catch all exceptions in ur_libapi.cpp

That is great, thanks a bunch for pointing it out! I admit I didn't know the loader already did that. I am keeping it in mind for future code because this certainly bloates the adapter source unnecessary if appropriate exception handling already exists in the libapi caller.

@omarahmed1111
Copy link
Contributor

@GeorgeWeb Could you please create an intel/llvm PR for this? Thanks!

@GeorgeWeb
Copy link
Contributor Author

@GeorgeWeb Could you please create an intel/llvm PR for this? Thanks!

@omarahmed1111 I can certainly add one. As a historical info, this has previously already been tested in a DPC++ PR and has made it in main alongside that PR. However, this UR change was mistakenly removed/deleted due to a rebase on another PR deleting it that was merged later on. Anyways, I can totally add a intel/llvm PR to ensure it still passes the pre-commit CI.

@omarahmed1111
Copy link
Contributor

@GeorgeWeb Could you please create an intel/llvm PR for this? Thanks!

@omarahmed1111 I can certainly add one. As a historical info, this has previously already been tested in a DPC++ PR and has made it in main alongside that PR. However, this UR change was mistakenly removed/deleted due to a rebase on another PR deleting it that was merged later on. Anyways, I can totally add a intel/llvm PR to ensure it still passes the pre-commit CI.

Thanks for updating me! an Intel/llvm PR will be needed not only for testing purpose but to bump UR to the commit of merging this PR.

…ct creation

Reintroduces the changes from commit c4ae460, which were reverted
in related merged commit 1b4a8b8 due to being mistakenly deleted
and omitted on rebasing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA adapter specific issues ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants