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.

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][Test] Track amount of inter-dependencies between headers #16202

Open
wants to merge 1 commit into
base: sycl
Choose a base branch
from

Conversation

AlexeySachkov
Copy link
Contributor

To improve compile-time, we may one day split sycl/sycl.hpp into separate headers, but we need to make sure that those headers don't include every other header so that there is an actual benefit of using them.

This PR introduced a test to track that certain headers which we already consider isolated are not used from anywhere else except from sycl/sycl.hpp.

Note that for now we allow extension headers to include everything and only focusing on the core SYCL headers.

To improve compile-time, we may one day split `sycl/sycl.hpp` into
separate headers, but we need to make sure that those headers don't
include every other header so that there is an actual benefit of using
them.

This PR introduced a test to track that certain headers which we already
consider isolated are not used from anywhere else except from
`sycl/sycl.hpp`.

Note that for now we allow extension headers to include everything and
only focusing on the core SYCL headers.
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.

👍

@@ -0,0 +1,27 @@
// sycl/sycl.hpp is a very heavy header, because it includes all SYCL feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// sycl/sycl.hpp is a very heavy header, because it includes all SYCL feature.
// sycl/sycl.hpp is a very heavy header, because it includes all SYCL features.

// unnecessary to make sure that when you include a single specific header, you
// only get specific functionality and nothing extra.
//
// Note that this test ignores "ext" subfolder for now and concentrates on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Note that this test ignores "ext" subfolder for now and concentrates on
// NOTE: This test ignores "ext" subfolder for now and concentrates on

Following FIXME: or TODO: notation, I find it helpful to use NOTE: to highlight the importance of this section and simplify the search.

Comment on lines +18 to +26
// RUN: grep -rl "#include <sycl/sub_group.hpp>" %sycl_include/sycl \
// RUN: --exclude-dir=*ext* | FileCheck %s --check-prefix=SUB-GROUP
// SUB-GROUP: sycl/sycl.hpp
// SUB-GROUP-EMPTY:
//
// RUN: grep -rl "#include <sycl/stream.hpp>" %sycl_include/sycl \
// RUN: --exclude-dir=*ext* | FileCheck %s --check-prefix=STREAM
// STREAM: sycl/sycl.hpp
// STREAM-EMPTY:
Copy link
Contributor

Choose a reason for hiding this comment

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

If I get the idea right, we check that #include <sycl/sub_group.hpp> is used only by sycl/sycl.hpp header. I guess this applies to any header declaring SYCL core class, right? If so, do we need to have separate matchers for each header?

Suggested change
// RUN: grep -rl "#include <sycl/sub_group.hpp>" %sycl_include/sycl \
// RUN: --exclude-dir=*ext* | FileCheck %s --check-prefix=SUB-GROUP
// SUB-GROUP: sycl/sycl.hpp
// SUB-GROUP-EMPTY:
//
// RUN: grep -rl "#include <sycl/stream.hpp>" %sycl_include/sycl \
// RUN: --exclude-dir=*ext* | FileCheck %s --check-prefix=STREAM
// STREAM: sycl/sycl.hpp
// STREAM-EMPTY:
// RUN: grep -rl "#include <sycl/sub_group.hpp>" %sycl_include/sycl \
// RUN: --exclude-dir=*ext* | FileCheck %s
// RUN: grep -rl "#include <sycl/stream.hpp>" %sycl_include/sycl \
// RUN: --exclude-dir=*ext* | FileCheck %s
//
// CHECK: sycl/sycl.hpp
// CHECK-EMPTY:

I guess we can further simplify the command to match all include patterns at once rather than searching one include per grep command.

@AlexeySachkov
Copy link
Contributor Author

👍

We actually had an offline talk with @aelovikov-intel here with conclusion that this test is not be a good idea :)

If I get the idea right, we check that #include <sycl/sub_group.hpp> is used only by sycl/sycl.hpp header.

That was the idea, right. Note that I'm specifically not taking extensions into account, they can include whatever they want at this point.

I guess this applies to any header declaring SYCL core class, right?

The thing is, SYCL extensions can add extra methods into core SYCL classes. Depending on what such a method is, it may require a full sub_group definition, thus breaking the test. It is not entirely clear what is the course of actions to fix this test then?

Even if we discard extensions, there are tons of inter-dependencies between core SYCL classes and it will be tricky to resolve them all by forward-declarations (see reduction.hpp and atomic_ref.hpp as an example).

We probably could say that stream should not be included anywhere else directly, but it will be hard to say that in general about every header that we have. For example. even after #16012 kernel_bundle.hpp will still be included from one more place (outside of ext/) besides sycl/sycl.hpp.

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.

2 participants