[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager #905

kswiecicki · 2023-09-28T13:25:50Z

This PR contains cherry-picked commits from #630.

jandres742 · 2023-09-28T19:09:40Z

This PR contains cherry-picked commits from #630.

so should we wait for #630 to be merged?

source/adapters/level_zero/context.cpp

jandres742 · 2023-09-28T19:11:13Z

Please open draft PR in intel/llvm for pre-merge testing, as indicated here https://github.com/oneapi-src/unified-runtime/pull/902/files

kswiecicki · 2023-09-29T12:04:09Z

Please open draft PR in intel/llvm for pre-merge testing, as indicated here https://github.com/oneapi-src/unified-runtime/pull/902/files

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

igchor · 2023-09-29T17:30:46Z

source/adapters/level_zero/context.hpp

-  // Store the host memory pool. It does not depend on any device.
-  umf::pool_unique_handle_t HostMemPool;
+  usm::pool_manager<usm::pool_descriptor> PoolManager;
+  usm::pool_manager<usm::pool_descriptor> ProxyPoolManager;


I'm wondering if it wouldn't be better to just have a flag in pool_descriptor to decide if we do any pooling. We could then have only a single pool manager.

But is it fitting to place such an option in the pool_descriptor?

Why not? I think in the future, we will want to have even more options in pool_descriptor, e.g. related to access characteristics, etc. that would influence pooling.

I agree, this looks odd. If you decide to do otherwise, please add a comment.

source/adapters/level_zero/context.cpp

source/adapters/level_zero/usm.cpp

source/adapters/level_zero/command_buffer.cpp

source/adapters/level_zero/usm.cpp

EwanC

I've only reviewed source/adapters/level_zero/command_buffer.cpp, but LGTM

kswiecicki · 2023-11-07T12:15:54Z

I've only reviewed source/adapters/level_zero/command_buffer.cpp, but LGTM

Hey @EwanC, because of the scope of this PR I have created a separate PR for the min/max call fix #1045, could you take a look?

fabiomestre · 2023-12-05T16:55:23Z

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.

codecov-commenter · 2023-12-15T12:02:10Z

Codecov Report

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

Comparison is base (5d5c810) 15.44% compared to head (174e2e4) 15.44%.
Report is 186 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     #905      +/-   ##
==========================================
- Coverage   15.44%   15.44%   -0.01%     
==========================================
  Files         239      239              
  Lines       33970    33970              
  Branches     3758     3758              
==========================================
- Hits         5246     5245       -1     
- Misses      28673    28674       +1     
  Partials       51       51

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

pbalcer · 2024-01-24T10:09:46Z

source/adapters/level_zero/context.cpp

@@ -178,111 +183,120 @@ UR_APIEXPORT ur_result_t UR_APICALL urContextSetExtendedDeleter(
  return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
 }

+// Template helper function for creating USM pools for given pool descriptor.
+template <typename P, typename... Args>
+std::pair<ur_result_t, umf::pool_unique_handle_t>


There's now a helper for this sort of case: https://github.com/oneapi-src/unified-runtime/blob/main/source/ur/ur.hpp#L277

I don't know if there's a benefit to using it since I call umf::poolMakeUnique that returns a pair anyway.

pbalcer · 2024-01-24T10:10:40Z

source/adapters/level_zero/context.cpp

+      return (Desc.deviceReadOnly) ? usm::DisjointPoolMemType::SharedReadOnly
+                                   : usm::DisjointPoolMemType::Shared;
+    default:
+      // Added to suppress 'not all control paths return a value' warning.


ur::unreachable() ?

pbalcer · 2024-01-24T10:14:47Z

source/adapters/level_zero/context.hpp

-  // Store the host memory pool. It does not depend on any device.
-  umf::pool_unique_handle_t HostMemPool;
+  usm::pool_manager<usm::pool_descriptor> PoolManager;
+  usm::pool_manager<usm::pool_descriptor> ProxyPoolManager;


I agree, this looks odd. If you decide to do otherwise, please add a comment.

pbalcer · 2024-01-24T10:16:24Z

@kswiecicki please create an intel/llvm PR to test the changes.

kswiecicki · 2024-01-24T10:18:42Z

@kswiecicki please create an intel/llvm PR to test the changes.

There's one already: intel/llvm#11357

... to get the UR_USM_ALLOC_INFO_POOL alloc info.

kswiecicki mentioned this pull request Sep 28, 2023

[SYCL][UR][L0] Access UMF pool handles through through usm::pool_manager intel/llvm#11238

Closed

jandres742 reviewed Sep 28, 2023

View reviewed changes

source/adapters/level_zero/context.cpp Outdated Show resolved Hide resolved

kswiecicki mentioned this pull request Sep 29, 2023

[SYCL][UR][L0] Update L0 adapter sources intel/llvm#11357

Closed

igchor reviewed Sep 29, 2023

View reviewed changes

kswiecicki force-pushed the adapters-l0-pool-manager branch 2 times, most recently from 87f4bb1 to 1357d8b Compare October 2, 2023 11:00

kswiecicki force-pushed the adapters-l0-pool-manager branch from 7298939 to 3af465b Compare October 24, 2023 10:55

kswiecicki requested review from a team as code owners October 24, 2023 10:55

kswiecicki force-pushed the adapters-l0-pool-manager branch 3 times, most recently from 7785d65 to 9bd5d0e Compare October 26, 2023 13:35

PatKamin reviewed Nov 6, 2023

View reviewed changes

source/adapters/level_zero/context.cpp Outdated Show resolved Hide resolved

source/adapters/level_zero/context.cpp Show resolved Hide resolved

source/adapters/level_zero/usm.cpp Outdated Show resolved Hide resolved

EwanC reviewed Nov 6, 2023

View reviewed changes

source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved

PatKamin reviewed Nov 6, 2023

View reviewed changes

source/adapters/level_zero/usm.cpp Show resolved Hide resolved

PatKamin mentioned this pull request Nov 6, 2023

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

Open

EwanC approved these changes Nov 7, 2023

View reviewed changes

EwanC mentioned this pull request Nov 7, 2023

[SYCL][UR][L0] Distinguish min/max calls from macros #1045

Merged

kswiecicki force-pushed the adapters-l0-pool-manager branch from 9bd5d0e to d74a357 Compare November 14, 2023 12:08

PatKamin approved these changes Nov 14, 2023

View reviewed changes

igchor approved these changes Nov 14, 2023

View reviewed changes

kswiecicki force-pushed the adapters-l0-pool-manager branch from d74a357 to 0c9ca70 Compare November 15, 2023 09:49

jandres742 approved these changes Nov 15, 2023

View reviewed changes

kswiecicki force-pushed the adapters-l0-pool-manager branch 2 times, most recently from 227801c to 71cac2c Compare November 22, 2023 09:58

fabiomestre changed the base branch from adapters to main December 5, 2023 16:55

kswiecicki force-pushed the adapters-l0-pool-manager branch from 71cac2c to 0e1b8db Compare December 15, 2023 10:48

kswiecicki force-pushed the adapters-l0-pool-manager branch from 0e1b8db to 5d76778 Compare December 20, 2023 09:34

kswiecicki force-pushed the adapters-l0-pool-manager branch from 967903d to b424494 Compare December 27, 2023 14:10

kswiecicki force-pushed the adapters-l0-pool-manager branch from b424494 to 25b9f9b Compare January 8, 2024 08:07

kswiecicki force-pushed the adapters-l0-pool-manager branch 2 times, most recently from e39523a to 7bf047d Compare January 23, 2024 12:51

pbalcer reviewed Jan 24, 2024

View reviewed changes

kswiecicki force-pushed the adapters-l0-pool-manager branch 3 times, most recently from 2093065 to 174e2e4 Compare February 1, 2024 09:36

kswiecicki added 5 commits February 5, 2024 10:38

[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager

85c25cd

[SYCL][UR][L0] Remove reduntant umf2urResult function

d251654

[SYCL][UR][L0] Add failure logs for UMF pools

fb98b75

[SYCL][UR][L0] Use usm::pool_manager functionality

e874284

... to get the UR_USM_ALLOC_INFO_POOL alloc info.

[SYCL][UR][L0] Distinguish std::min call from macro

350a5dc

kswiecicki force-pushed the adapters-l0-pool-manager branch from 174e2e4 to 350a5dc Compare February 5, 2024 09:38

kbenzie added common Changes or additions to common utilities level-zero L0 adapter specific issues labels Apr 10, 2024

igchor mentioned this pull request Apr 18, 2024

Revisit the necessity the various control knobs in L0 adapter #1528

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager #905

[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager #905

kswiecicki commented Sep 28, 2023

jandres742 commented Sep 28, 2023

jandres742 commented Sep 28, 2023

kswiecicki commented Sep 29, 2023

igchor Sep 29, 2023

kswiecicki Oct 3, 2023

igchor Oct 26, 2023

pbalcer Jan 24, 2024

EwanC left a comment

kswiecicki commented Nov 7, 2023

fabiomestre commented Dec 5, 2023

codecov-commenter commented Dec 15, 2023 •

edited

Loading

pbalcer Jan 24, 2024

kswiecicki Jan 26, 2024

pbalcer Jan 24, 2024

kswiecicki Jan 26, 2024

pbalcer Jan 24, 2024

pbalcer commented Jan 24, 2024

kswiecicki commented Jan 24, 2024

[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager #905

Are you sure you want to change the base?

[SYCL][UR][L0] Access UMF pool handles through usm::pool_manager #905

Conversation

kswiecicki commented Sep 28, 2023

jandres742 commented Sep 28, 2023

jandres742 commented Sep 28, 2023

kswiecicki commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EwanC left a comment

Choose a reason for hiding this comment

kswiecicki commented Nov 7, 2023

fabiomestre commented Dec 5, 2023

codecov-commenter commented Dec 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbalcer commented Jan 24, 2024

kswiecicki commented Jan 24, 2024

codecov-commenter commented Dec 15, 2023 •

edited

Loading