Skip to content

[NFCI] Introduce <sycl/detail/core.hpp> include #12890

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
merged 4 commits into from
Mar 21, 2024

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Mar 1, 2024

The idea is to have a thinner version of <sycl/sycl.hpp> that allows to run basic tests and enables fine-grained includes in our e2e tests. Hopefully that can increase CI efficiency.

@bader
Copy link
Contributor

bader commented Mar 1, 2024

Could you run something like:

echo "#include <sycl/sycl.hpp>" > sycl.cpp
clang++ -c -fsycl -MD sycl.cpp -MF sycl.deps
echo "#include <sycl/core.hpp>" > core.cpp
clang++ -c -fsycl -MD core.cpp -MF core.deps

and compare sycl.deps vs core.deps?

From my experiments in the past, I noticed that just queue.hpp implicitly includes ~70% of SYCL headers.
Another interesting experiment is to dump pre-processed files (use -E -P) to compare how much code we can save using core.hpp instead of sycl.hpp.

I would really want to document core.hpp as oneAPI, extension so it can be used by the users (not just tests).

@aelovikov-intel
Copy link
Contributor Author

and compare sycl.deps vs core.deps?

$ wc -l *deps
  437 core.deps
  557 sycl.deps
  994 total

@aelovikov-intel aelovikov-intel marked this pull request as ready for review March 19, 2024 00:05
@aelovikov-intel aelovikov-intel requested review from a team as code owners March 19, 2024 00:05
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

Whilst I support activities to speed up our tests (see my previous attempts in #7438), I have a concern about modifying E2E tests: they now can't be used as examples of SYCL usage, because they all contain non-standard undocumented functionality which can easily confuse our users.

I don't see any documentation being updated:

  • it will be a bit harder (i.e. less obvious) for someone who haven't seen the patch to figure out what's going on
  • we will have to explain this separately to everyone who is asking questions
  • new tests will still be added with sycl/sycl.hpp unless someone thoroughly reviews all patches

@aelovikov-intel
Copy link
Contributor Author

Whilst I support activities to speed up our tests (see my previous attempts in #7438), I have a concern about modifying E2E tests: they now can't be used as examples of SYCL usage, because they all contain non-standard undocumented functionality which can easily confuse our users.

I don't see any documentation being updated:

  • it will be a bit harder (i.e. less obvious) for someone who haven't seen the patch to figure out what's going on
  • we will have to explain this separately to everyone who is asking questions
  • new tests will still be added with sycl/sycl.hpp unless someone thoroughly reviews all patches

I'm afraid this is a chicken and an egg problem. We can't make a meaningful documentation update (e.g. an official extension) until we've experimented enough and found out what we can reasonably achieve.

On the other hand, I doubt anybody could work on this full-time and not being able to commit small incremental pieces would result in most of the time spent in rebases/merges slowing down any possible progress even further. I personally expect myself working on this not more than a few hours a week.

I think the best we can have at the time is an informal discussion/direction alignment (like comments in this PR) and then anybody could keep making minor contribution until we have a better understanding of what works and what not. As a result, I think

new tests will still be added with sycl/sycl.hpp unless someone thoroughly reviews all patches

is completely ok for the time being. Also, we already have some tests that only include individual headers and not the entire <sycl/sycl.hpp>, so I'm not really making a major break.

Copy link
Contributor

@cperkinsintel cperkinsintel left a comment

Choose a reason for hiding this comment

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

I think this is great and should have been done from the beginning.

But I do think we should document it, even if just as an experimental extension. If it's useful to us, it's likely to be useful to other users.

The idea is to have a thinner version of `<sycl/sycl.hpp>` that allows
to run basic tests and enables fine-grained includes in our e2e tests.
Hopefully that can increase CI efficiency.
@aelovikov-intel aelovikov-intel changed the title [NFCI] Introduce <sycl/core.hpp> include [NFCI] Introduce <sycl/detail/core.hpp> include Mar 20, 2024
@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Mar 20, 2024

@intel/syclcompat-lib-reviewers , @intel/dpcpp-esimd-reviewers , ping.

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.

No additional flags from ESIMD

@sarnex sarnex requested a review from a team March 21, 2024 14:50
@aelovikov-intel aelovikov-intel merged commit b7501ae into intel:sycl Mar 21, 2024
@aelovikov-intel aelovikov-intel deleted the sycl-core branch March 21, 2024 21:10
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request May 20, 2024
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request May 20, 2024
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request May 20, 2024
aelovikov-intel added a commit that referenced this pull request May 20, 2024
aelovikov-intel added a commit that referenced this pull request May 20, 2024
aelovikov-intel added a commit that referenced this pull request May 21, 2024
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