-
Notifications
You must be signed in to change notification settings - Fork 735
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
Conversation
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.
LGTM
Why don't we need a test for/with this? |
I think we do need a test. I have added a simple test. |
Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>
Added test extended_deleters/extended_deleters.cpp is failing in CI for some reasons. Could you please address?
|
This reverts commit 9aa3a27.
Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
@intel/dpcpp-l0-pi-reviewers @intel/llvm-reviewers-runtime please review ASAP, this is the start of a PR dependency chain. |
Ping @intel/llvm-gatekeepers |
// REQUIRES: hip, cuda | ||
|
||
// RUN: %{build} -o %t.out | ||
// RUN: %{run} %t.out | FileCheck %s | ||
|
||
// CHECK: This extended deleter should be called at ctx destruction. | ||
|
||
#include <sycl/sycl.hpp> | ||
|
||
int main() { | ||
sycl::context c; | ||
sycl::detail::pi::contextSetExtendedDeleter( | ||
c, | ||
[](void *) { | ||
printf("This extended deleter should be called at ctx destruction."); | ||
}, | ||
nullptr); | ||
} |
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.
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.
something that is used in oneMKL through the sycl::detail::pi interface.
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.
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'm not excited about this change, but I also don't want to have this discussion here.
A typo in #10514 got rid of the extended deleter callbacks. This reinstates them.