[SYCL] Add test for extended deleters #11344

hdelan · 2023-09-28T14:11:59Z

A typo in #10514 got rid of the extended deleter callbacks. This reinstates them.

npmiller

LGTM

sycl/plugins/unified_runtime/ur/adapters/hip/context.cpp

aelovikov-intel · 2023-09-28T19:06:20Z

Why don't we need a test for/with this?

hdelan · 2023-09-29T09:23:28Z

Why don't we need a test for/with this?

I think we do need a test. I have added a simple test.

sycl/test/extended_deleters/extended_deleters.cpp

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>

againull · 2023-10-09T19:17:13Z

Added test extended_deleters/extended_deleters.cpp is failing in CI for some reasons. Could you please address?

# RUN: at line 1
%{build} -o D:\github\_work\llvm\llvm\build\tools\sycl\test\extended_deleters\Output\extended_deleters.cpp.tmp.out
# executed command: '%{build}' -o 'D:\github\_work\llvm\llvm\build\tools\sycl\test\extended_deleters\Output\extended_deleters.cpp.tmp.out'
# .---command stderr------------
# | '%{build}': command not found
# `-----------------------------
# error: command failed with exit status: 127

This reverts commit c8267b1.

sycl/plugins/unified_runtime/CMakeLists.txt

This reverts commit 9aa3a27.

sycl/plugins/unified_runtime/CMakeLists.txt

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>

kbenzie · 2023-10-20T14:19:52Z

@intel/dpcpp-l0-pi-reviewers @intel/llvm-reviewers-runtime please review ASAP, this is the start of a PR dependency chain.

hdelan · 2023-10-20T14:55:27Z

Ping @intel/llvm-gatekeepers

aelovikov-intel · 2023-10-20T15:03:36Z

sycl/test-e2e/ExtendedDeleters/extended_deleters.cpp

+// 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);
+}


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.

If the test does move to UR it'll need to be in a separate PR.

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.

@aelovikov-intel see here https://github.com/oneapi-src/unified-runtime/blob/main/test/conformance/context/urContextSetExtendedDeleter.cpp

something that is used in oneMKL through the sycl::detail::pi interface.

That is bad... detail shouldn't be used in production.

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 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

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.

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.

aelovikov-intel

I'm not excited about this change, but I also don't want to have this discussion here.

Re add extended deleters

5a13e4d

hdelan requested a review from a team as a code owner September 28, 2023 14:12

hdelan requested a review from jchlanda September 28, 2023 14:12

hdelan temporarily deployed to WindowsCILock September 28, 2023 14:12 — with GitHub Actions Inactive

hdelan had a problem deploying to WindowsCILock September 28, 2023 14:34 — with GitHub Actions Failure

npmiller approved these changes Sep 28, 2023

View reviewed changes

npmiller reviewed Sep 28, 2023

View reviewed changes

sycl/plugins/unified_runtime/ur/adapters/hip/context.cpp Outdated Show resolved Hide resolved

Revert delete call

3460ade

hdelan temporarily deployed to WindowsCILock September 28, 2023 16:04 — with GitHub Actions Inactive

npmiller approved these changes Sep 28, 2023

View reviewed changes

hdelan temporarily deployed to WindowsCILock September 28, 2023 16:27 — with GitHub Actions Inactive

Add extended deleters test

310e8d0

hdelan requested a review from a team as a code owner September 29, 2023 09:23

hdelan requested a review from againull September 29, 2023 09:23

hdelan had a problem deploying to WindowsCILock September 29, 2023 09:23 — with GitHub Actions Failure

hdelan temporarily deployed to WindowsCILock September 29, 2023 09:46 — with GitHub Actions Inactive

aelovikov-intel reviewed Sep 29, 2023

View reviewed changes

sycl/test/extended_deleters/extended_deleters.cpp Outdated Show resolved Hide resolved

Update sycl/test/extended_deleters/extended_deleters.cpp

c8267b1

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>

hdelan had a problem deploying to WindowsCILock September 29, 2023 15:49 — with GitHub Actions Failure

hdelan temporarily deployed to WindowsCILock September 29, 2023 16:55 — with GitHub Actions Inactive

jchlanda approved these changes Oct 3, 2023

View reviewed changes

jle-quel mentioned this pull request Oct 4, 2023

[LAPACK][rocSOLVER] Update rocSOLVER backend oneapi-src/oneMKL#393

Merged

2 tasks

hdelan added 2 commits October 10, 2023 10:22

Revert "Update sycl/test/extended_deleters/extended_deleters.cpp"

c7d258b

This reverts commit c8267b1.

Remove change to adapter

0dc75c2

hdelan had a problem deploying to WindowsCILock October 10, 2023 09:29 — with GitHub Actions Failure

hdelan mentioned this pull request Oct 10, 2023

[HIP] Re add extended deleters oneapi-src/unified-runtime#943

Merged

hdelan changed the title ~~[SYCL][HIP] Re add extended deleters~~ [SYCL] Add test for extended deleters Oct 10, 2023

hdelan temporarily deployed to WindowsCILock October 13, 2023 14:43 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock October 13, 2023 15:03 — with GitHub Actions Inactive

againull reviewed Oct 13, 2023

View reviewed changes

sycl/plugins/unified_runtime/CMakeLists.txt Outdated Show resolved Hide resolved

Update commit SHA in UR repo

0f6ee56

hdelan temporarily deployed to WindowsCILock October 17, 2023 13:26 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock October 17, 2023 13:47 — with GitHub Actions Inactive

kbenzie mentioned this pull request Oct 17, 2023

[UR][L0] Propagate errors from USMAllocationMakeResident #11312

Closed

Update SHA

9aa3a27

hdelan temporarily deployed to WindowsCILock October 19, 2023 11:43 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock October 19, 2023 12:02 — with GitHub Actions Inactive

Revert "Update SHA"

b2993ce

This reverts commit 9aa3a27.

hdelan temporarily deployed to WindowsCILock October 19, 2023 12:58 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock October 19, 2023 13:18 — with GitHub Actions Inactive

Merge branch 'sycl' into fix-typo-in-hip-adapter

f2489be

hdelan temporarily deployed to WindowsCILock October 20, 2023 10:30 — with GitHub Actions Inactive

Point SHA to reverted commit branch

8d05088

hdelan temporarily deployed to WindowsCILock October 20, 2023 10:48 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock October 20, 2023 11:11 — with GitHub Actions Inactive

kbenzie reviewed Oct 20, 2023

View reviewed changes

sycl/plugins/unified_runtime/CMakeLists.txt Outdated Show resolved Hide resolved

Update sycl/plugins/unified_runtime/CMakeLists.txt

9788c42

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>

hdelan temporarily deployed to WindowsCILock October 20, 2023 11:20 — with GitHub Actions Inactive

hdelan temporarily deployed to WindowsCILock October 20, 2023 11:42 — with GitHub Actions Inactive

fabiomestre mentioned this pull request Oct 20, 2023

[SYCL][OpenCL] Fetch the adapter source from the UR repo #11566

Merged

ldrumm approved these changes Oct 20, 2023

View reviewed changes

aelovikov-intel reviewed Oct 20, 2023

View reviewed changes

aelovikov-intel approved these changes Oct 20, 2023

View reviewed changes

againull approved these changes Oct 20, 2023

View reviewed changes

againull merged commit 2def4d8 into intel:sycl Oct 20, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add test for extended deleters #11344

[SYCL] Add test for extended deleters #11344

hdelan commented Sep 28, 2023

npmiller left a comment

aelovikov-intel commented Sep 28, 2023

hdelan commented Sep 29, 2023

againull commented Oct 9, 2023

kbenzie commented Oct 20, 2023

hdelan commented Oct 20, 2023

aelovikov-intel Oct 20, 2023

kbenzie Oct 20, 2023

hdelan Oct 20, 2023

hdelan Oct 20, 2023

aelovikov-intel Oct 20, 2023 •

edited

Loading

hdelan Oct 20, 2023

hdelan Oct 20, 2023

AlexeySachkov Oct 23, 2023

hdelan Oct 23, 2023

aelovikov-intel left a comment

[SYCL] Add test for extended deleters #11344

[SYCL] Add test for extended deleters #11344

Conversation

hdelan commented Sep 28, 2023

npmiller left a comment

Choose a reason for hiding this comment

aelovikov-intel commented Sep 28, 2023

hdelan commented Sep 29, 2023

againull commented Oct 9, 2023

kbenzie commented Oct 20, 2023

hdelan commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelovikov-intel Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelovikov-intel left a comment

Choose a reason for hiding this comment

aelovikov-intel Oct 20, 2023 •

edited

Loading