Skip to content

[SYCL] Make ur::getAdapter return raw adapter pointer instead of shared_ptr #19102

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

Open
wants to merge 5 commits into
base: sycl
Choose a base branch
from

Conversation

uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Jun 23, 2025

Currently, ur::getAdapter returns a shared pointer to the adapter. However, that's unnecessary.
This PR makes ur::getAdapter return a raw ptr instead and let global handler manage lifetime of the adapter object.

@uditagarwal97 uditagarwal97 self-assigned this Jun 23, 2025
@uditagarwal97 uditagarwal97 requested a review from vinser52 June 23, 2025 18:15
@uditagarwal97 uditagarwal97 marked this pull request as ready for review June 23, 2025 18:15
@uditagarwal97 uditagarwal97 requested review from a team as code owners June 23, 2025 18:15
@uditagarwal97 uditagarwal97 requested a review from slawekptak June 23, 2025 18:15
@uditagarwal97
Copy link
Contributor Author

Followup PR to pass adapter by ref instead of pointer: #19105

@@ -230,12 +230,22 @@ std::mutex &GlobalHandler::getFilterMutex() {
return FilterMutex;
}

std::vector<AdapterPtr> &GlobalHandler::getAdapters() {
static std::vector<AdapterPtr> &adapters = getOrCreate(MAdapters);
std::vector<std::unique_ptr<Adapter>> &GlobalHandler::getAdapters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this method? Why is the getAdapterRawPtrs not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, GlobalHandler::getAdapters() is still required. It is used in ur::initializeUr to pass by reference the MAdapter data member, which is later used to initialize this variable in initializeAdapters().

@@ -126,7 +126,7 @@ class GlobalHandler {
InstWithLock<std::mutex> MPlatformToDefaultContextCacheMutex;
InstWithLock<std::mutex> MPlatformMapMutex;
InstWithLock<std::mutex> MFilterMutex;
InstWithLock<std::vector<AdapterPtr>> MAdapters;
InstWithLock<std::vector<std::unique_ptr<Adapter>>> MAdapters;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we agree that we do not need the getAdapters and keep only the getAdapterRawPtrs, which returns std::vector<Adapter *>, then this line can be changed to the InstWithLock<std::vector<Adapter*>> and we can use a custom deleter when constructing the MAdapters.

Copy link
Contributor

Choose a reason for hiding this comment

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

And you will not need to change the signature of the ur::initializeUr() to return std::vector by copy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants