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

[L0] Manage UMF pools through usm::pool_manager #2495

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[L0] Manage UMF pools through usm::pool_manager #2495

wants to merge 3 commits into from

Conversation

kswiecicki
Copy link
Contributor

No description provided.

@github-actions github-actions bot added common Changes or additions to common utilities level-zero L0 adapter specific issues labels Dec 20, 2024
@kswiecicki kswiecicki force-pushed the l0-pool-manager branch 5 times, most recently from b00072c to 2a62983 Compare January 8, 2025 11:46
@github-actions github-actions bot added the ci/cd Continuous integration/devliery label Jan 8, 2025
@kswiecicki kswiecicki force-pushed the l0-pool-manager branch 2 times, most recently from 281e3e4 to 30d5386 Compare January 13, 2025 11:55
@kswiecicki kswiecicki marked this pull request as ready for review January 13, 2025 15:45
@kswiecicki kswiecicki requested review from a team as code owners January 13, 2025 15:45
igchor
igchor reviewed Jan 13, 2025
View reviewed changes
source/common/ur_pool_manager.hpp Outdated
@@ -289,6 +289,12 @@ template <typename D> struct pool_manager {

return it->second.get();
}

bool hasPool(umf_memory_pool_handle_t hPool) noexcept {
Copy link
Member

Choose a reason for hiding this comment

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

One UR pulls down this commit oneapi-src/unified-memory-framework@86d2341 hasPool won't be necessary - you will be able to just use umfPoolByPtr

Copy link
Contributor Author

@kswiecicki kswiecicki Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've decided not to include the umfPool[Set/Get]Tag changes in this PR. After trying to implement it I've encountered some complications in ur_result_t ur_validation_layer::bounds(ur_queue_handle_t, const void*, size_t, size_t) from ur_validation_layer.cpp.

@kswiecicki kswiecicki mentioned this pull request Jan 14, 2025
Draft
@kswiecicki kswiecicki mentioned this pull request Jan 15, 2025
Closed
@kswiecicki kswiecicki force-pushed the l0-pool-manager branch 3 times, most recently from 44fcaa0 to 51fec4f Compare January 21, 2025 14:25
@kswiecicki kswiecicki force-pushed the l0-pool-manager branch 2 times, most recently from 8a2b671 to 38752a1 Compare January 27, 2025 10:28
@kswiecicki kswiecicki force-pushed the l0-pool-manager branch 3 times, most recently from b7a42f9 to 9bc686d Compare February 4, 2025 13:35
@kswiecicki kswiecicki force-pushed the l0-pool-manager branch 4 times, most recently from 8023b0a to 5ee6a87 Compare February 10, 2025 17:49
@kswiecicki kswiecicki requested review from pbalcer and igchor February 13, 2025 14:58
pbalcer
pbalcer reviewed Feb 14, 2025
View reviewed changes
source/adapters/level_zero/memory.cpp

// Peform actual device allocation as needed.
if (!Allocation.ZeHandle) {
if (!SingleRootDeviceBufferMigration && UrContext->SingleRootDevice &&
Copy link
Contributor

Choose a reason for hiding this comment

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

did you intentionally remove this logic?
why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SingleRootDevice from Context is default initialized to nullptr and is never set to any value, hence this if statement would always evaluate to false so I've removed this variable alltogether.

igchor
igchor reviewed Feb 14, 2025
View reviewed changes
source/adapters/level_zero/usm.cpp Outdated
// find the allocator depending on context as we do for Shared and Device
// allocations.
umf_memory_pool_handle_t hPoolInternal = nullptr;
ur_usm_pool_handle_t USMPool = nullptr;
if (!UseUSMAllocator) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't UseUSMAllocator a global variable? Can't we just read it in contextCreate and decide whether to create ProxyPool or DisjointPool there? We would only have one defaultPool in the context this way.

Copy link
Contributor Author

@kswiecicki kswiecicki Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Done. I've removed the ProxyPool from Context, now it's the DefaultPool that uses different pool implementation depending on the UseUSMAllocator variable.

@kswiecicki kswiecicki requested a review from a team as a code owner February 18, 2025 11:32
@github-actions github-actions bot added the command-buffer Command Buffer feature addition/changes/specification label Feb 18, 2025
@kswiecicki
[L0] Manage UMF pools through usm::pool_manager
5d7c2e7
@kswiecicki
[L0] Remove SingleRootDevice from context
a4456ac 
This variable is not initialized in any way and it is set to nullptr by
default.
@kswiecicki
[L0] Fix provider native error reporting
69b3a4e 
Level Zero provider internally stores native errors of ur_result_t type.
@kswiecicki kswiecicki mentioned this pull request Feb 19, 2025
Merged
@martygrant
Copy link
Contributor

Unified Runtime -> intel/llvm Repo Move Notice

Information

The source code of Unified Runtime has been moved to intel/llvm under the unified-runtime top-level directory,
all future development will now be carried out there. This was done in intel/llvm#17043.

The code will be mirrored to oneapi-src/unified-runtime and the specification will continue to be hosted at oneapi-src.github.io/unified-runtime.

The contribution guide has been updated with new instructions for contributing to Unified Runtime.

PR Migration

All open PRs including this one will be labelled auto-close and shall be automatically closed after 30 days.
To allow for some breathing space, this automation will not be enabled until next week (27/02/2025).

Should you wish to continue with your PR you will need to migrate it to intel/llvm.
We have provided a script to help automate this process.

This is an automated comment.

@martygrant
Copy link
Contributor

Unified Runtime -> intel/llvm Repo Move Notice

Following on from the previous notice, we have now enabled workflows to automatically label and close PRs because the Unified Runtime source code has moved to intel/llvm.

This PR has now been marked with the auto-close label and will be automatically closed after 30 days.

Please review the previous notice for more information, including assistance with migrating your PR to intel/llvm.

Should there be a reason for this PR to remain open, manually remove the auto-close label.

This is an automated comment.

uditagarwal97 pushed a commit to intel/llvm that referenced this pull request Mar 10, 2025
@kswiecicki
[UR][L0] Manage UMF pools through usm::pool_manager (#17065)
38d7506 
PR moved from: oneapi-src/unified-runtime#2495
RossBrunton pushed a commit that referenced this pull request Mar 10, 2025
@kswiecicki @RossBrunton
Manage UMF pools through usm::pool_manager (#17065)
5b6c754 
PR moved from: #2495
adamfidel pushed a commit to reble/llvm that referenced this pull request Mar 11, 2025
@kswiecicki @adamfidel
[UR][L0] Manage UMF pools through usm::pool_manager (intel#17065)
a45e961 
PR moved from: oneapi-src/unified-runtime#2495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbalcer pbalcer pbalcer left review comments

@igchor igchor igchor left review comments

@konradkusiak97 konradkusiak97 Awaiting requested review from konradkusiak97 konradkusiak97 was automatically assigned from oneapi-src/unified-runtime-command-buffer-write

Assignees
No one assigned
Labels
auto-close ci/cd Continuous integration/devliery command-buffer Command Buffer feature addition/changes/specification common Changes or additions to common utilities level-zero L0 adapter specific issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kswiecicki @martygrant @pbalcer @igchor