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.

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

[SYCL][UR][CUDA] Access UMF pool handles through usm::pool_manager #957

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

[SYCL][UR][CUDA] Access UMF pool handles through usm::pool_manager #957

wants to merge 1 commit into from

Conversation

kswiecicki
Copy link
Contributor

This PR contains cherry-picked commits from #630.

@kswiecicki kswiecicki mentioned this pull request Oct 12, 2023
Draft
@kswiecicki
Copy link
Contributor Author

Draft for pre-merge testing: intel/llvm#11523

@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 2 times, most recently from 6b9b0f0 to a44ab49 Compare October 26, 2023 10:56
@kswiecicki kswiecicki marked this pull request as ready for review October 26, 2023 10:56
@kswiecicki kswiecicki requested review from a team as code owners October 26, 2023 10:56
@kswiecicki kswiecicki mentioned this pull request Oct 26, 2023
Draft
@PatKamin
Copy link
Contributor

PatKamin commented Nov 6, 2023

See my comments in #905 , they also apply here.

@kswiecicki
Copy link
Contributor Author

See my comments in #905 , they also apply here.

I've made similar changes to CUDA adapter.

@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 2 times, most recently from 1a86450 to 4ba968e Compare November 20, 2023 09:13
@PatKamin
Copy link
Contributor

#630 is merged. Please, rebase.

@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 2 times, most recently from a3798b6 to d61e267 Compare November 22, 2023 09:57
@fabiomestre fabiomestre changed the base branch from adapters to main December 5, 2023 16:53
@fabiomestre
Copy link
Contributor

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 2 times, most recently from a63a56a to a393b47 Compare December 20, 2023 09:34
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f15234d) 14.83% compared to head (34d722a) 14.84%.
Report is 3 commits behind head on main.

Files Patch % Lines
source/common/ur_pool_manager.hpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #957   +/-   ##
=======================================
  Coverage   14.83%   14.84%           
=======================================
  Files         250      250           
  Lines       36232    36232           
  Branches     4097     4097           
=======================================
+ Hits         5376     5377    +1     
+ Misses      30805    30804    -1     
  Partials       51       51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 2 times, most recently from b6f0a7f to 39074a8 Compare January 23, 2024 12:53
@kswiecicki
Copy link
Contributor Author

Hey @aarongreig, could you take a look at this PR?

npmiller
npmiller requested changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

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

Minor comment fixes, but looks good overall

source/adapters/cuda/usm.cpp Outdated
false};
auto hPoolInternalOpt = hPool->PoolManager.getPool(Desc);
if (!hPoolInternalOpt.has_value()) {
// Internal error, every L0 context and usm pool should have Host, Device,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Internal error, every L0 context and usm pool should have Host, Device,
// Internal error, every CUDA context and usm pool should have Host, Device,

Should this say CUDA instead of L0? There's a few other comments below referencing L0 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

aarongreig
aarongreig approved these changes Jan 25, 2024
View reviewed changes
source/adapters/cuda/usm.cpp Show resolved Hide resolved
source/adapters/cuda/usm.cpp Outdated Show resolved Hide resolved
@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 6 times, most recently from 76b93cf to 899b53e Compare February 1, 2024 09:30
@kswiecicki kswiecicki force-pushed the adapters-cuda-pool-manager branch 2 times, most recently from 4f15997 to 83c8cd4 Compare February 13, 2024 14:39
@kbenzie kbenzie added common Changes or additions to common utilities cuda CUDA adapter specific issues labels Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongreig aarongreig aarongreig approved these changes

@npmiller npmiller npmiller requested changes

@igchor igchor igchor approved these changes

@PatKamin PatKamin PatKamin approved these changes

Assignees
No one assigned
Labels
common Changes or additions to common utilities cuda CUDA adapter specific issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kswiecicki @PatKamin @fabiomestre @codecov-commenter @npmiller @aarongreig @igchor @kbenzie