-
Notifications
You must be signed in to change notification settings - Fork 117
Dynamic Selection Adding ResourceAdapter to support queue* #2367
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
Dynamic Selection Adding ResourceAdapter to support queue* #2367
Conversation
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for resource adapters in the dynamic selection framework, allowing users to provide a resource_adapter
alongside a resource
where the resulting base_resource = resource_adapter(resource)
defines the default backend implementation. This enables use cases like using sycl::queue*
as the resource type with a dereference adapter to reduce copying overhead and enable pointer-based lookup patterns.
Key Changes
- Adds
ResourceAdapter
template parameter to policy classes and backend implementations - Updates test utilities to support passing adapter arguments to policy constructors
- Extends test coverage to include pointer-based resource scenarios with dereference adapters
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/support/test_dynamic_selection_utils.h | Updated test utility functions to accept variadic arguments for adapter support |
test/support/test_dynamic_load_utils.h | Enhanced dynamic load test utilities with adapter support and templated initialization |
test/parallel_api/dynamic_selection/sycl/test_round_robin_policy_sycl.pass.cpp | Added tests for both direct sycl::queue and sycl::queue* with dereference adapter |
test/parallel_api/dynamic_selection/sycl/test_fixed_resource_policy_sycl.pass.cpp | Extended tests to cover pointer-based resources with adapters |
test/parallel_api/dynamic_selection/sycl/test_dynamic_load_policy_sycl.pass.cpp | Added comprehensive testing for dynamic load policy with adapter support |
include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h | Refactored to support ResourceAdapter template parameter and adapter-based resource handling |
include/oneapi/dpl/internal/dynamic_selection_impl/round_robin_policy.h | Added ResourceAdapter template parameter with default std::identity |
include/oneapi/dpl/internal/dynamic_selection_impl/policy_base.h | Updated initialization to support variadic arguments for adapter passing |
include/oneapi/dpl/internal/dynamic_selection_impl/fixed_resource_policy.h | Enhanced with ResourceAdapter support and updated constructor signatures |
include/oneapi/dpl/internal/dynamic_selection_impl/dynamic_load_policy.h | Added ResourceAdapter template parameter and updated policy instantiation |
include/oneapi/dpl/internal/dynamic_selection_impl/default_backend.h | Restructured with default_backend_impl base class and ResourceAdapter support |
Comments suppressed due to low confidence (1)
include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h
Outdated
Show resolved
Hide resolved
include/oneapi/dpl/internal/dynamic_selection_impl/sycl_backend.h
Outdated
Show resolved
Hide resolved
Signed-off-by: Dan Hoeflinger <[email protected]>
ResourceAdapter __adapter; | ||
}; | ||
|
||
template <typename ResourceType, typename ResourceAdapter = std::identity> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like std::identity is c++20, are we able to implement its functionality for use with c++17?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we have an alternative in oneDPL we can use.
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
This PR allows users to provide a
resource_adapter
alongside aresource
where the resultingbase_resource = resource_adapter(resource)
defines the default backend implementation.This enables a use case where a user may want a resource to contain more or even a different variant as compared to the core base resource which defines a backend implementation.
The motivating use case is using
sycl::queue*
as the resource type alongside an adapter which dereferences. This allows less overhead copying pointers rather than full queues. It also allows the pointer to be used as a unique identifier to do things like look up a memory space in a global map.Alternatively, you could envision a resource being provided which is a
std::pair<sycl::queue, usm_pointer>
, and an adapter functorstd::get<0>
to solve a similar use case (without the potential benefit of reducing overhead).