-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Make handler.hpp
independent from kernel_bundle.hpp
#16012
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] Make handler.hpp
independent from kernel_bundle.hpp
#16012
Conversation
// CHECK-NEXT: kernel_bundle.hpp | ||
// CHECK-NEXT: ext/oneapi/experimental/free_function_traits.hpp |
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.
Any removals here are good and desirable, I think :)
Also, feel free to add any new tests here (e.g. for kernel_bundle.hpp
itself) if you'd find that beneficial to highlight the improvements (either this PR or any other of the same spirit).
template <bundle_state State> | ||
class kernel_bundle; |
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 been thinking about providing sycl/[detail/]fwd/<something.hpp>
includes for quite some time, but always proceeded without it. Do you find such an idea attractive? My main concern was structure/granularity for such forward declarations.
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.
As I was working on #16030 and putting even more forward-declarations everywhere, I though that having a dedicated version of every header which only provides forward declarations would actually be cool.
Because some classes are templated, their forward-declaration may not be trivial and it may require more forward-declarations. Having a ready-to-use "canonical" forward-declaration in form of a separate header would shorten the codebase and make it simpler (i.e. that would just be a #include
list without any extra lines of code).
There is a thing that some headers provide multiple classes and functions and therefore we could "import" more forward-declarations than we need, but I assume that it would be cheaper anyway that including the original header, so I don't think that it is a strong enough point against the idea.
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.
esimd/tools lgtm
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.
Joint Matrix changes LGTM
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.
👍 SYCLcompat changes LGTM
I see some unexpected failures during local merge with |
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 guess i can do this to prevent accidental automerge, ping me/gatekeepers when the PR is ready
I was looking into ways of splitting
sycl.hpp
into different finer-grained headers (with the intention to propose such split as a KHR extension/SYCL-Next thing) and I decided to try and see what is the impact of different header files on the compilation time.I started my investigation with
kernel_bundle.hpp
. Looking at zjin-lcf/HeCBench, I do not see any benchmarks that use it, so it seems like a good candidate for being an opt-in header.To do the measurements I decided to drop
#include <kernel_bundle.hpp>
fromsycl.hpp
and then compare compilation time of two empty files includingsycl.hpp
(the modified one and the original one). Apparently, it is not that easy to drop an include, because there are so many inter-dependencies on it. I succeeded and I see ~200ms device compilation time improvement whenkernel_bundle.hpp
is not included at all.However, for my experiments I made some other hacks which I'm unable to push into the repo, like dropping backend-specific headers as well. Specifically, L0 backend interop has
kernel_bundle
as a struct member of some of input or return types which requires a full definition.SYCL spec (6.3.7. Adding a backend) allows backend interop headers to be put into separate headers and I think that we should actually use this opportunity in the future and drop them (somehow without many regressions) from
sycl.hpp
.