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.

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

Jump to bottom

[SYCL][L0] Avoid PI kernel leak for SYCL-L0 interop kernel #6980

Merged
merged 1 commit into from
Oct 11, 2022
Merged

[SYCL][L0] Avoid PI kernel leak for SYCL-L0 interop kernel #6980

merged 1 commit into from
Oct 11, 2022

Conversation

smaslov-intel
Copy link
Contributor

Signed-off-by: Sergey V Maslov sergey.v.maslov@intel.com

@smaslov-intel
[SYCL][L0] Avoid PI kernel leak for SYCL-L0 interop kernel
d99b20b 
Signed-off-by: Sergey V Maslov <sergey.v.maslov@intel.com>
@smaslov-intel smaslov-intel requested a review from a team as a code owner October 6, 2022 16:09
smaslov-intel
smaslov-intel commented Oct 6, 2022
View reviewed changes
sycl/source/detail/kernel_impl.cpp
// This constructor is only called in the interoperability kernel constructor.
// Let the runtime caller handle native kernel retaining in other cases if
// it's needed.
getPlugin().call<PiApiKind::piKernelRetain>(MKernel);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proper retain (for OpenCL only) is done in the "make_kernel" that calls this constructor:

llvm/sycl/source/backend.cpp

Line 258 in 1c3d598

Plugin.call<PiApiKind::piKernelRetain>(PiKernel);

romanovvlad
romanovvlad approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

Please, add a test to sycl/unittests/

@smaslov-intel
Copy link
Contributor Author

Please, add a test to sycl/unittests/

There is already an E2E test in llvm-test-suite/SYCL/OnlineCompiler/online_compiler_L0.cpp, which found the problem.
I don't think we need to spend time to create a unittest for this

@smaslov-intel
Copy link
Contributor Author

Note, the ESIMD test failure cannot be related to this PR:

/__w/llvm/llvm/toolchain/bin/../include/sycl/ext/intel/esimd/detail/types_elementary.hpp:86:34: error: invalid vector element type 'sycl::_V1::ext::intel::experimental::esimd::tfloat32'
using type = Ty attribute((ext_vector_type(N)));

@romanovvlad
Copy link
Contributor

Please, add a test to sycl/unittests/

There is already an E2E test in llvm-test-suite/SYCL/OnlineCompiler/online_compiler_L0.cpp, which found the problem. I don't think we need to spend time to create a unittest for this

Could you please clarify if the test fails without the patch? If not, probably we need to improve it/write a better one.

@smaslov-intel
Copy link
Contributor Author

Please, add a test to sycl/unittests/

There is already an E2E test in llvm-test-suite/SYCL/OnlineCompiler/online_compiler_L0.cpp, which found the problem. I don't think we need to spend time to create a unittest for this

Could you please clarify if the test fails without the patch? If not, probably we need to improve it/write a better one.

Yes, the test fails without this patch when run in valgrind.

@smaslov-intel
Copy link
Contributor Author

@againull , @bader : can this be merged, please?

@pvchupin pvchupin merged commit b083e05 into intel:sycl Oct 11, 2022
steffenlarsen added a commit to steffenlarsen/llvm that referenced this pull request Oct 12, 2022
@steffenlarsen
[SYCL] Fix retaining of kernels in old kernel interop
5d767fa 
intel#6980 caused some unexpected
regressions on OpenCL for SYCL 1.2.1 kernel interop constructors. This
commit reintroduces the call to retaining the OpenCL kernel only for the
old constructor.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen mentioned this pull request Oct 12, 2022
Merged
pvchupin pushed a commit that referenced this pull request Oct 12, 2022
@steffenlarsen
[SYCL] Fix retaining of kernels in old kernel interop (#7034)
a32021b 
#6980 caused some unexpected
regressions on OpenCL for SYCL 1.2.1 kernel interop constructors. This
commit reintroduces the call to retaining the OpenCL kernel only for the
old constructor.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@smaslov-intel smaslov-intel deleted the kernel_leak branch October 17, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanovvlad romanovvlad romanovvlad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smaslov-intel @romanovvlad @pvchupin