Skip to content

[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

Merged

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Nov 7, 2024

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> from sycl.hpp and then compare compilation time of two empty files including sycl.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 when kernel_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.

Comment on lines -162 to -136
// CHECK-NEXT: kernel_bundle.hpp
// CHECK-NEXT: ext/oneapi/experimental/free_function_traits.hpp
Copy link
Contributor

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

Comment on lines 43 to 44
template <bundle_state State>
class kernel_bundle;
Copy link
Contributor

@aelovikov-intel aelovikov-intel Nov 7, 2024

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.

Copy link
Contributor Author

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.

@AlexeySachkov AlexeySachkov marked this pull request as ready for review November 21, 2024 09:43
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner November 21, 2024 09:43
@AlexeySachkov AlexeySachkov requested review from a team as code owners November 22, 2024 10:45
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

esimd/tools lgtm

Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a 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

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@Alcpz Alcpz left a comment

Choose a reason for hiding this comment

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

👍 SYCLcompat changes LGTM

@AlexeySachkov
Copy link
Contributor Author

I see some unexpected failures during local merge with sycl branch, in case some gatekeeper would want to merge the PR: please don't do that just yet. I will merge it myself after I re-check post-merge results.

Copy link
Contributor

@sarnex sarnex left a 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

@AlexeySachkov
Copy link
Contributor Author

i guess i can do this to prevent accidental automerge, ping me/gatekeepers when the PR is ready

Failures seem to be a side-effect of #15905 and unrelated to this PR, I can reproduce them on pure sycl branch as well. We can proceed with merge here, @sarnex

@sarnex sarnex merged commit 13ff78e into intel:sycl Dec 2, 2024
14 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/dont-include-kernel-bundle branch December 3, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants