Add common pool handle and memory provider classes to the common umf helpers #1248
Conversation
|
@kswiecicki obviously this patch isn't finished (especially on the l0 side) but just a heads up that I started working on adding pool support to the CL adapter and due to the greater need there to support arbitrary allocation flags I ended up making a bunch of changes to the UMF helper stuff in |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
+ Coverage 15.73% 15.76% +0.02%
==========================================
Files 223 224 +1
Lines 31478 31573 +95
Branches 3558 3563 +5
==========================================
+ Hits 4954 4977 +23
- Misses 26473 26545 +72
Partials 51 51 ☔ View full report in Codecov by Sentry. |
aarongreig
force-pushed
the
aaron/USMPoolsCommon
branch
3 times, most recently
from
ca61b08
to
1d81e64
Compare
... to get the UR_USM_ALLOC_INFO_POOL alloc info.
Also port cuda and level zero adapters to make use of these (these are the adapters that were previously been updated to use the pool manager changes that this patch relies on).
aarongreig
force-pushed
the
aaron/USMPoolsCommon
branch
from
1d81e64
to
dff4988
Compare
| @@ -158,63 +194,83 @@ inline bool pool_descriptor::operator==(const pool_descriptor &other) const { | |||
| } | |||
| } | |||
|
|
|||
| if (lhs.allocLocation.has_value()) { | |||
There was a problem hiding this comment.
nit: I think you can simplify it to: if (lhs.allocLocation != rhs.allocLocation) return false;
There was a problem hiding this comment.
Ah, nevermind, pNext might be different. I'm wondering if we should perhaps implement operator== for ur_usm_alloc_location_desc_t ...
| } | ||
| } | ||
|
|
||
| return {ret, descriptors}; | ||
| } | ||
|
|
||
| inline usm::DisjointPoolMemType | ||
| urTypeToDisjointPoolType(const ur_usm_type_t &type) { |
There was a problem hiding this comment.
I think only having these three types makes sense because they correspond to the three allocation entry points in UR, read only pools will still be distinct by the fact that they're TYPE_SHARED and pass the appropriate read only alloc flag
There was a problem hiding this comment.
The problem is that Shared and SharedReadOnly pools have different defaults (specifically, for Shared pooling is disabled by default) and we also allow users to change the pooling behavior for each of those 4 types separately through UR_L0_USM_ALLOCATOR (aka SYCL_PI_LEVEL_ZERO_USM_ALLOCATOR). This env variable is not very well documented and is still considered 'experimental' but it's been useful for some applications.
There was a problem hiding this comment.
Oh I see.. I was worried that the extra type was more than it first seemed. This aspect probably needs a re-design in that case, I'll be revisiting this PR once the pool manager PRs merge and I can rebase
| throw umf::UsmAllocationException(Ret); | ||
| } | ||
|
|
||
| PoolManager.addPool(desc, newPool); |
There was a problem hiding this comment.
nit: I think it would be less error-prone to make addPool accept uniquq_ptr by value so that the caller would need to std::move() the argument.
No description provided.