[SYCL] Make handler.hpp independent from kernel_bundle.hpp
#16012
Conversation
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
| // CHECK-NEXT: kernel_bundle.hpp | ||
| // CHECK-NEXT: ext/oneapi/experimental/free_function_traits.hpp |
There was a problem hiding this comment.
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.
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.
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.
sycl/include/sycl/handler.hpp
Outdated
| // This is somewhat radical, but to make handler.hpp independtent from | ||
| // kernel_bundle.hpp, we define those methods within kernel_bundle.hpp | ||
| // header. Independence is needed in context of potential upcoming split of | ||
| // sycl.hpp so that users could do fine-grained include's, saving on | ||
| // compilation time by avoiding using headers for features they don't use. |
There was a problem hiding this comment.
I've done the same in a few places already for previous splits. I think mine are in queue.hpp
| // The purpose of this test is to ensure that the header containing | ||
| // sycl::handler class definition is self-contained, i.e. we can use handler | ||
| // and no extra headers are needed. |
There was a problem hiding this comment.
Can we have tighter integration with your generic tests for self-contained-headers? Something like not generating generic test for a header if it has a dedicated specific one (like this).
There was a problem hiding this comment.
There was a problem hiding this comment.
Outlined this upgrade into #16117
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
AlexeySachkov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
I was looking into ways of splitting
sycl.hppinto 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.hppand 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.hppis not included at all.The cost is high, however and there are several questionable changes in this PR. I do not plan to submit it as-is: it is mostly to showcase the work and discuss the approach. But I do plan to submit some changes from this PR, because they seem like a good cleanup in general.
The most questionable moments:
handler.hppuseskernel_bundlein specialization-constant related methods. For now, I simply moved their implementation intokernel_bundle.hpp, but that is confusing for both developers and end users (will it be required to include a separate header to use ahandler::set_specialization_constantmethod?). I believe thatsetcan be refactored so that it operates directly onkernel_bundle_impl, but I'm not so sure aboutget.kernel_bundleincludes from various backend-interop headers. 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 and drop them (somehow without many regressions) fromsycl.hpp