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][NFC] Refactor #includes #16030

Open
wants to merge 10 commits into
base: sycl
Choose a base branch
from

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Nov 8, 2024

This patch is a collection of various cleanups made in public headers:

  • Cleaned up many unnecessary includes. It doesn't change total amount of header files we use in total by sycl.hpp, but makes our code cleaner
  • Made it so there are no headers (except for backend/%backend_name%.hpp) depend on backend.hpp and it is (almost) only included by sycl.hpp, so that we can make it an opt-in header
  • Removed types.hpp in favor of direct use of vector.hpp
  • Added missing includes and forward-declarations to places where we relied on implicit includes
  • Moved certain helper function declarations/definitions to better places (common utils to utils headers, library-only declarations to library headers, etc.)

This patch is a collection of various cleanups made in public headers:
- Cleaned up many unnecessary includes. It doesn't change total amount
  of header files we use in total by `sycl.hpp`, but makes our code
  cleaner
- Made it so there are no headers depending on `backend.hpp` and it is
  only included by `sycl.hpp`, so that we can make it an opt-in header
- Removed `types.hpp` in favor of direct use of `vector.hpp`
- Added missing includes and forward-declarations to places where we
  relied on implicit includes
- Moved certain helper function declarations/definitions to better
  places (common utils to utils headers, library-only declarations to
  library headers, etc.)
Copy link
Contributor Author

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

I will make the patch ready for review, even though CI fails: by some reason checkout step started to consistently fail for my PR - I will try to figure that our while I'm waiting for feedback and applying it.

sycl/include/sycl/vector.hpp Outdated Show resolved Hide resolved
@@ -15,7 +15,6 @@ set(SYCL_THREADS_LIB ${CMAKE_THREAD_LIBS_INIT})

# TEST_INCLUDE_PATH is used for syntax-only verification of type information.
list(APPEND test_includes ${SYCL_INCLUDE})
list(APPEND test_includes ${SYCL_INCLUDE}/sycl)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: without this change test/gdb/printer.cpp failed to compile. The reason for that is because in program_impl.hpp we have #include <detail/ur.hpp> line.

We have two detail/ur.hpp headers: one under sycl/include/sycl and another under sycl/source. During the RT build we only have sycl/include and sycl/source include paths, so we take the right header and everything works fine.
However, for this specific test we add an extra include path, which makes a wrong file to be included and leads to missing declaration of convertUrBackend (which is moved into the library by this PR). I do not see any failures which would be caused by this particular change and I also don't quite understand why do we need this path at all.

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.

lgtm

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.

4 participants