Skip to content

[SYCL] Add test for extended deleters #11344

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Oct 20, 2023
Merged

[SYCL] Add test for extended deleters #11344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5a13e4d
Re add extended deleters
Sep 28, 2023
3460ade
Revert delete call
Sep 28, 2023
310e8d0
Add extended deleters test
Sep 29, 2023
c8267b1
Update sycl/test/extended_deleters/extended_deleters.cpp
hdelan Sep 29, 2023
c7d258b
Revert "Update sycl/test/extended_deleters/extended_deleters.cpp"
Oct 10, 2023
0dc75c2
Remove change to adapter
Oct 10, 2023
bbd1e52
Move test to test-e2e
Oct 13, 2023
650bb36
Change UR repo and sha
Oct 13, 2023
11b9d83
Merge branch 'sycl' into fix-typo-in-hip-adapter
hdelan Oct 13, 2023
d0286d9
Only run for HIP, CUDA
Oct 13, 2023
0f6ee56
Update commit SHA in UR repo
Oct 17, 2023
9aa3a27
Update SHA
Oct 19, 2023
b2993ce
Revert "Update SHA"
Oct 19, 2023
f2489be
Merge branch 'sycl' into fix-typo-in-hip-adapter
Oct 20, 2023
8d05088
Point SHA to reverted commit branch
Oct 20, 2023
9788c42
Update sycl/plugins/unified_runtime/CMakeLists.txt
hdelan Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions sycl/plugins/unified_runtime/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ if(SYCL_PI_UR_USE_FETCH_CONTENT)
include(FetchContent)

set(UNIFIED_RUNTIME_REPO "https://github.com/oneapi-src/unified-runtime.git")
#commit b38855ed815ffd076bfde5e5e06170ca4f723dc1
#Merge: e6343f4 6a2c548
#Author: Piotr Balcer <piotr.balcer@intel.com>
#Date: Thu Oct 5 12:15:42 2023 +0200
# Merge pull request #920 from jsji/localcopy
# [UR][L0] Copy prebuilt L0 to avoid leaking shared folder path
set(UNIFIED_RUNTIME_TAG b38855ed815ffd076bfde5e5e06170ca4f723dc1)
# commit a76e3b185a5e1df9114dc483077227f88f522244
# Merge: 3653e582 657ffde9
# Author: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
# Date: Fri Oct 20 12:12:15 2023 +0100
# Merge pull request #972 from oneapi-src/revert-906-l0_usm_error_checking
# Revert "[UR][L0] Propagate errors from `USMAllocationMakeResident`"
set(UNIFIED_RUNTIME_TAG a76e3b185a5e1df9114dc483077227f88f522244)

if(SYCL_PI_UR_OVERRIDE_FETCH_CONTENT_REPO)
set(UNIFIED_RUNTIME_REPO "${SYCL_PI_UR_OVERRIDE_FETCH_CONTENT_REPO}")
Expand Down
18 changes: 18 additions & 0 deletions sycl/test-e2e/ExtendedDeleters/extended_deleters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// 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);
}
Comment on lines +1 to +18
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

@aelovikov-intel aelovikov-intel Oct 20, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I've submitted uxlfoundation/oneMath#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.

Copy link
Contributor Author

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.