[SYCL] Fix include loop and use-before-def #8407
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are two issues present here that aren't obviously visible:
1. The `sycl::half_type` alias isn't defined in sycl/half_type.hpp; it
actually comes from detail/generic_type_lists.hpp seemingly
accidentally.
2. detail/type_traits.hpp transitively includes sycl/half_type.hpp as
it has templates that use sycl::half. However, the half
implementation itself needs type_traits.hpp for computing
alignment. Depending on the order of headers included, this causes
an unfulfillable declaration-before-use issue in client code. In
doing so ESIMD was found to be guilty of not including all the
correct headers.
There are many such transitive header issues lurking which keep the
headers tightly coupled and the final preprocess very large. Until they
are solved in a more general way I've had to introduce some copy-paste
for the `vector_alignment` helper to avoid including type_traits.hpp
inside half_type.hpp.
This patch is a prerequisite of the future complex_half group algorithms
work ending up in a working order
ldrumm
force-pushed
the
half-include-order
branch
from
a8982c1 to
e4b3590
Compare
detail/helpers.hpp was needed by spirv_ops.cpp for the inline definition of detail::memcpy, but including such caused a circular ordering issue. There's no need to forward declare an inline function that's immediately needed, so hoist it into its own header that can easily be included in both users.
|
If anyone has bandwidth to look at this I'd appreciate it; It's a prerequisite of making 7120 work |
|
@intel/llvm-gatekeepers |
steffenlarsen
approved these changes
Mar 3, 2023
|
@intel/dpcpp-esimd-reviewers - Friendly ping. |
|
On Fri Mar 3, 2023 at 3:08 PM GMT, Steffen Larsen wrote:
Would it make sense to move this to a new file, e.g.
`vector_traits.hpp`, similar to what you did with memcpy? Hopefully that
should avoid problems in case we ever need to change this.
Yeah. That seems sensible and more consistent. I'll update
|
steffenlarsen
approved these changes
Mar 3, 2023
ldrumm
force-pushed
the
half-include-order
branch
from
443255c to
06830e7
Compare
ldrumm
force-pushed
the
half-include-order
branch
from
06830e7 to
95728c8
Compare
|
Is it worth rerunning the HIP timeouts? |
Not at the moment. They are considered unsupported for now will be disabled with intel/llvm-test-suite#1639. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two issues present here that aren't obviously visible:
sycl::half_typealias isn't defined in sycl/half_type.hpp; it actually comes from detail/generic_type_lists.hpp seemingly accidentally.There are many such transitive header issues lurking which keep the headers tightly coupled and the final preprocess very large. Until they are solved in a more general way I've had to introduce some copy-paste for the
vector_alignmenthelper to avoid including type_traits.hpp inside half_type.hpp.This patch is a prerequisite of the future complex_half group algorithms work ending up in a working order