-
Notifications
You must be signed in to change notification settings - Fork 730
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][NFCI] Drop ESIMD emulator leftovers #15495
base: sycl
Are you sure you want to change the base?
[SYCL][NFCI] Drop ESIMD emulator leftovers #15495
Conversation
intel#4020 introduced some kind of wrapper for SYCL kernels on host to be able to launch them on ESIMD emulator backend. Even though we had dropped that feature since then, we didn't remove corresponding `handler.hpp` modifications. This PR removes them. The main effect expected from this PR is compilation time improvement: those kernel wrappers are specialized by kernel name, meaning that there will be plenty of extra useless functions emitted during host compilation pass for every kernel in a program. This patch also uncovered a missing case in `HostKernel::InstantiateKernelOnHost` which we couldn't ever encounter because we transformed a host kernel to always accept `nd_item`, thus always skipping problematic `item` code path. This PR is not expected to introduce any functional changes, but since I'm not very familiar with the SYCL RT, I'm not entirely sure of 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.
Changes LGTM overall. However, I'm not very familiar with how InstantiateKernelOnHost
is used, so, I'd request an additional review from @aelovikov-intel
sycl/include/sycl/handler.hpp
Outdated
KernelType *KernelPtr = | ||
ResetHostKernel<KernelType, LambdaArgType, Dims>(KernelFunc); | ||
MHostKernel.reset( | ||
new detail::HostKernel<KernelType, LambdaArgType, Dims>(KernelFunc)); |
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.
should it be = std::make_unique
?
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.
Originally I simply reverted this code to its older state, but applied make_unique
in ed3589e
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.
However, I'm not very familiar with how InstantiateKernelOnHost is used
Me neither :)
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.
@steffenlarsen should be able to share some insights about that method of HostKernel
, because he was the one who [re-?]introduced it not that long ago
sycl/include/sycl/handler.hpp
Outdated
KernelType *KernelPtr = | ||
ResetHostKernel<KernelType, LambdaArgType, Dims>(KernelFunc); | ||
MHostKernel.reset( | ||
new detail::HostKernel<KernelType, LambdaArgType, Dims>(KernelFunc)); |
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.
Originally I simply reverted this code to its older state, but applied make_unique
in ed3589e
#4020 introduced some kind of wrapper for SYCL kernels on host to be able to launch them on ESIMD emulator backend. Even though we had dropped that feature since then, we didn't remove corresponding
handler.hpp
modifications. This PR removes them.The main effect expected from this PR is compilation time improvement: those kernel wrappers are specialized by kernel name, meaning that there will be plenty of extra useless functions emitted during host compilation pass for every kernel in a program.
This patch also uncovered a missing case in
HostKernel::InstantiateKernelOnHost
which we couldn't ever encounter because we transformed a host kernel to always acceptnd_item
, thus always skipping problematicitem
code path.This PR is not expected to introduce any functional changes, but since I'm not very familiar with the SYCL RT, I'm not entirely sure of that.