[DO NOT MERGE] [SYCL][ABI break] Make sycl::vec trivially_copyable and align layout #6816
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
Before the patch the data sycl::vec used underneath was either cl_* types or clang vector types(aka ext_vector_type) depending on the host compiler. This was causing binary compatibility issues because such types are passed differently according to the SysV calling convention. The way such types are passed also depends on the instruction set a compiler allowed to use(aka -avx). The vector types were used underneath in order speed up sycl::vec operations on the host when available(when compiling with clang). So, to avoid performance degradation we need to continue doing the operations using vector types. The patch solves this problem by changing the underlying type to std::array in all the cases for the host compilation. And adds conversions to vector types when the host compiler is clang to avoid loosing performance. The GCC vector_size could be used to cover both compiler, but it has some problems to solve, e.g. it does not support vector of lenght 3. This also allows us to untie sycl::vec from OpenCL types. The are things to improve: 1. Use vectors data type in more sycl::vec methods 2. Use vectors for half type 3. Refactor several ifdef SYCL_DEVICE_ONLY to if constexpr constructs.
romanovvlad
changed the title
|
The pre-commit failed with very strange error. Cannot reproduce neither with clang nor with GCC 9.3. Anyway, adding an init to zero. |
romanovvlad
commented
Sep 20, 2022
| @@ -63,6 +63,8 @@ function(add_sycl_rt_library LIB_NAME LIB_OBJ_NAME) | |||
| else() | |||
| target_compile_options(${LIB_OBJ_NAME} PUBLIC | |||
| -fvisibility=hidden -fvisibility-inlines-hidden) | |||
|
|
|||
| target_compile_options(${LIB_OBJ_NAME} PUBLIC -Wno-psabi) | |||
There was a problem hiding this comment.
Add a comment:
Silencing GCC note about changed ABI in quite old version of GCC which is not supported by the product:
"the ABI for passing parameters with 32-byte alignment has changed in GCC 4.6"
BTW. For some reason I was not able to silence the note just using pragma in the files, so silencing it globally.
romanovvlad
changed the title
steffenlarsen
approved these changes
Sep 20, 2022
Speaking honestly, I don't have a good idea how to write such a test. We need to pass a sycl::vec object between two TUs compiled using clang and gcc to reproduce that. |
againull
pushed a commit
that referenced
this pull request
Aug 2, 2023
adapted from #6816 sycl::vec now uses std::array on host in all cases, but device access is via `ext_vector_type` so as to not lose performance. Alignment is set the same as the size to a maximum of 64 bytes, which aligns with an upcoming spec change proposal. In the cases where the size is greater than 64 we use std::array on the device. We no longer need to emit a pragma on Windows and we can use `alignas()` on all platforms.
mdtoguchi
pushed a commit
to mdtoguchi/llvm
that referenced
this pull request
Oct 18, 2023
adapted from intel#6816 sycl::vec now uses std::array on host in all cases, but device access is via `ext_vector_type` so as to not lose performance. Alignment is set the same as the size to a maximum of 64 bytes, which aligns with an upcoming spec change proposal. In the cases where the size is greater than 64 we use std::array on the device. We no longer need to emit a pragma on Windows and we can use `alignas()` on all platforms.
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.
Before the patch the data sycl::vec used underneath was either cl_* types or clang vector types(aka ext_vector_type) depending on the host compiler.
This was causing binary compatibility issues because such types are passed differently according to the SysV calling convention. The way such types are passed also depends on the instruction set a compiler allowed to use(aka -avx).
The vector types were used underneath in order speed up sycl::vec operations on the host when available(when compiling with clang). So, to avoid performance degradation we need to continue doing the operations using vector types.
The patch solves this problem by changing the underlying type to std::array in all the cases for the host compilation. And adds conversions to vector types when the host compiler is clang to avoid loosing performance. The GCC vector_size could be used to cover both compiler, but it has some problems to solve, e.g. it does not support vector of lenght 3.
This also allows us to untie sycl::vec from OpenCL types.
The are things to improve: