[SYCL][NFC] Update includes in DPC++ headers. #10648
Conversation
…emulator_device_interface.hpp.
bader
changed the title
bader
requested review from
a team,
victor-eds,
Naghasan and
sommerlukas
as code owners
There was a problem hiding this comment.
Additional notes for reviewers.
While working on this patch I relied on our tests, so I highly recommend to pay
close attention to the code that potentially could be cut by pre-processor. If
you see something unexpected, please, let me and code owners know, so we improve
the situation with dependencies.
I'm encourage you to look at includes and symbols imported from them and think
about either using forward declaration or (if possible) moving the code from
DPC++ headers to DPC++ runtime library.
Something I haven't mentioned in the code comments -
sycl/include/sycl/info/info_desc.hpp declares descriptors for all SYCL objects.
I think it better to split this header to have a one header with info descriptor
per SYCL object.
| @@ -8,12 +8,13 @@ | |||
|
|
|||
| #pragma once | |||
|
|
|||
| #include <sycl/id.hpp> | |||
| #include <sycl/access/access.hpp> // for mode, placeholder, target | |||
| #include <sycl/buffer.hpp> // for range | |||
There was a problem hiding this comment.
Hm... should it be
| #include <sycl/buffer.hpp> // for range | |
| #include <sycl/range.hpp> // for range |
?
🤔
| #include <sycl/detail/item_base.hpp> // for id, range | ||
| #include <sycl/id.hpp> // for id | ||
| #include <sycl/item.hpp> // for item | ||
| #include <sycl/range.hpp> // for range |
There was a problem hiding this comment.
Do we really need #include <sycl/detail/item_base.hpp>?
Based on my experience with the tool, I don't think we should expect contributors to run this tool locally. I suggest we don't change contribution policy. |
|
@intel/llvm-reviewers-runtime, ping. |
These changes were done in semi-automatic mode by [include what you use](https://github.com/include-what-you-use/include-what-you-use/) tool. The caveat is that this tool doesn't support SYCL compilation mode, so it analyses only host code and I had to manually adjust includes for the device compilation. The tool annotated each include with the symbols declared in the included header and used in the code. I tired to apply following structure to includes where it's possible: ```c++ // <copyright header> #pragma once #include "local/includes" #include <sycl/includes> #ifdef .. #include <conditional/includes> #endif #include <standard_library> ``` Removed sycl/include/sycl/detail/sycl_fe_intrins.hpp and sycl/include/sycl/detail/service_kernel_names.hpp - declarations from these headers are used only once, so there is sense in moving them to a separate headers. I've put a few comments for follow-up actions: 1. sycl/include/sycl/handler.hpp includes 57 headers, which seems to much. 2. sycl/include/sycl/info/info_desc.hpp and sycl/include/sycl/detail/info_desc_helpers.hpp - includes .def files, which depend on previous includes, doesn't sound like a good idea. 3. sycl/include/sycl/detail/kernel_desc.hpp - uses __SYCL_EXPORT, which doesn't sounds right to me, but I might be missing something.
These changes were done in semi-automatic mode by include what you
use tool. The
caveat is that this tool doesn't support SYCL compilation mode, so it analyses
only host code and I had to manually adjust includes for the device compilation.
The tool annotated each include with the symbols declared in the included header
and used in the code.
I tired to apply following structure to includes where it's possible:
Removed sycl/include/sycl/detail/sycl_fe_intrins.hpp and
sycl/include/sycl/detail/service_kernel_names.hpp - declarations from these headers
are used only once, so there is sense in moving them to a separate headers.
I've put a few comments for follow-up actions:
sycl/include/sycl/detail/info_desc_helpers.hpp - includes .def files, which
depend on previous includes, doesn't sound like a good idea.
sounds right to me, but I might be missing something.