Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SYCL] Add test for extended deleters #11344
[SYCL] Add test for extended deleters #11344
Changes from 10 commits
5a13e4d
3460ade
310e8d0
c8267b1
c7d258b
0dc75c2
bbd1e52
650bb36
11b9d83
d0286d9
0f6ee56
9aa3a27
b2993ce
f2489be
8d05088
9788c42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do we need it here and not in the UR repo? This makes no sense to me...
sycl::detail::pi
has to be eradicated from the sycl repo, IMO.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.
If the test does move to UR it'll need to be in a separate PR.
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.
We already have a test for this in the UR repo but this is something that is used in oneMKL through the
sycl::detail::pi
interface. I think it's worthwhile having a test to make sure that the SYCL interface is working as well as the UR one.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.
@aelovikov-intel see here https://github.com/oneapi-src/unified-runtime/blob/main/test/conformance/context/urContextSetExtendedDeleter.cpp
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.
That is bad...
detail
shouldn't be used in production.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.
I agree but I think that ship has sailed. See here https://github.com/oneapi-src/oneMKL/blob/develop/src/blas/backends/cublas/cublas_scope_handle.cpp#L123
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.
There have been some changes in UR contexts of late, and when these are settled we are going to re-review this entry point and see if we can potentially remove it from oneMKL, but as it stands it is necessary
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.
I've submitted oneapi-src/oneMKL#401. Personally, I'm in favor of removing the test: we do not guarantee availability of the tested functionality. The fact that it resides in
sycl::detail
namespace explicitly communicates that.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.
I think we should remove this test whenever we stop using this in oneMKL. The reason why I am adding this test is because this functionality was accidentally removed, and since there was no testing for it we didn't realise that something was broken, and oneMKL ended up breaking.