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.

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

[UR] Add handles to opencl adapter #17572

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

RossBrunton
Copy link
Contributor

No description provided.

@RossBrunton
Copy link
Contributor Author

This was from oneapi-src/unified-runtime#1176 . It flagged up some issues in LLVM which needed to be fixed (which they have now been).

@RossBrunton RossBrunton marked this pull request as ready for review March 26, 2025 11:17
@RossBrunton RossBrunton requested review from a team as code owners March 26, 2025 11:17
: CLEvent(Event), Context(Ctx), Queue(Queue) {
RefCount = 1;
urContextRetain(Context);
if (Queue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than allowing Queue to be null I think we should query it out of Event here, otherwise we're introducing a regression when you query EVENT_INFO_QUEUE for an event created from a native handle since the implementation for that query just returns hEvent->Queue

Copy link
Contributor

Choose a reason for hiding this comment

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

actually that doesn't work because the query should return the corresponding ur handle not the native one, not sure what to do about this but it is a regression

Comment on lines +128 to 133
size_t refCount = hContext->getReferenceCount();
// ExtFuncPtrCache is destroyed in an atexit() callback, so it doesn't
// necessarily outlive the adapter (or all the contexts).
if (refCount == 1 && cl_ext::ExtFuncPtrCache) {
cl_ext::ExtFuncPtrCache->clearCache(clContext);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could do fewer atomic accesses here

Suggested change
size_t refCount = hContext->getReferenceCount();
// ExtFuncPtrCache is destroyed in an atexit() callback, so it doesn't
// necessarily outlive the adapter (or all the contexts).
if (refCount == 1 && cl_ext::ExtFuncPtrCache) {
cl_ext::ExtFuncPtrCache->clearCache(clContext);
}
if (hContext->decrementReferenceCount() == 0 ) {
// ExtFuncPtrCache is destroyed in an atexit() callback, so it doesn't
// necessarily outlive the adapter (or all the contexts).
if(cl_ext::ExtFuncPtrCache) {
cl_ext::ExtFuncPtrCache->clearCache(clContext);
}
delete hContext;
}


std::string SupportedExtensions(ExtStr.c_str());
if (ExtStr.find("cl_khr_command_buffer") != std::string::npos) {
SupportedExtensions += " ur_exp_command_buffer";
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't report exp features via extension any more

cl_adapter::cast<cl_event *>(phEvent)));

NumberOfQueues, &CLQueue, hCommandBuffer->CLCommandBuffer,
numEventsInWaitList, CLWaitEvents.data(), &Event));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if we could avoid creating a cl_event if the user hasn't passed a phEvent, this is the kind of in-order queue use-case there's an effort to optimize DPC++ for at the moment.

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.

4 participants