Details
- Reviewers
philnik - Group Reviewers
Restricted Project - Commits
- rGa284d0cc9c69: [libcxx] <experimental/simd> Added aliagned flag types, traits…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/CMakeLists.txt | ||
---|---|---|
882 | Looks like you forgot to update the status page. | |
libcxx/include/experimental/__simd/aligned_tag.h | ||
36 | Let's add __assume_aligned in __memory/assume_aligned.h to avoid using builtins randomly in the code. This lets us also assert if the alignment assumption isn't met (Please add a test for that!). | |
libcxx/include/experimental/__simd/traits.h | ||
80 | Same here - does this even work with variables and this needs a SFINAE test. | |
80 | Isn't this implicit through is_simd_mask_v? | |
libcxx/include/experimental/__simd/utility.h | ||
24–30 | let's instead add a __bit_ceil for pre-C++20 usage. | |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
26 | Also the comment here - _LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES is libc++-specific, so it shouldn't be used in a stdtest. | |
libcxx/test/std/experimental/simd/test_utils.h | ||
22 | I don't think this is really an action item. The experimental implementation will probably be C++17 until we remove it. Only once it's (hopefully) landed in C++26 we can move the implementation and tests out of experimental and refactor any code. | |
23 | Since you already mentioned that they are equivalent. |
libcxx/include/experimental/__simd/traits.h | ||
---|---|---|
80 | I don't think so. _Up should be able to be provided by the user. _Tp::value_type is just a default type. |
libcxx/include/experimental/__simd/aligned_tag.h | ||
---|---|---|
36 | If I understand correctly, what you want is a false test when provides incorrect alignment. Then both tests in this patch do not involve alignment inputs. Can we add these false tests in subsequent patches about constructors and load/store that involved with alignment inputs? |
libcxx/include/experimental/__simd/aligned_tag.h | ||
---|---|---|
36 | Oh yeah, sure. We can test it through the public interface. | |
libcxx/include/experimental/__simd/traits.h | ||
80 | Ah, I missed that _Up may be provided by the user. Never mind then. | |
libcxx/include/experimental/__simd/utility.h | ||
22–39 | Let's add these in their respective implementation detail headers. That way we avoid code duplication and can use them in other places in the codebase. | |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
27 | Again, to make sure the value is static. | |
29–30 | It looks to me like this is technically implementation-defined. Am I correct in that interpretation? | |
39–40 | I think this is technically also the case for float and double. It just happens to be the case that they are the IEEE types on about every platform. | |
libcxx/test/std/experimental/simd/test_utils.h | ||
66 | Newline! |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
---|---|---|
29–30 | If my understanding of the document is correct, the basic type of the simd_mask is bool, while the storage type in the internal implementation layer is implementation-defined. Alignment should be calculated using bool. |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
---|---|---|
39–40 | is there any other modify required here? |
Please rebase. It looks like the CI didn't run properly.
libcxx/docs/Status/ParallelismProjects.csv | ||
---|---|---|
7 ↗ | (On Diff #539989) | There doesn't seem to be a [parallel.simd.aligned] section. AFAICT this is just [parallel.simd.synopsis]. |
libcxx/include/__bit/bit_ceil.h | ||
25 ↗ | (On Diff #539989) | Otherwise you're lying. |
libcxx/include/__memory/assume_aligned.h | ||
26 ↗ | (On Diff #539989) | This will break C++03 otherwise. |
libcxx/include/experimental/__simd/aligned_tag.h | ||
35 | ||
libcxx/include/experimental/__simd/traits.h | ||
86 | I think this might actually be wrong. The wording says that memory_alignment shouldn't contain value, not that the class should fail to instantiate. | |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
29–30 | I can't find any wording that suggests a required alignment. Consider a platform with a 32byte vector. If the platform can load that vector from a 16byte aligned address without any loss of performance, there is no reason to make memory_alignment return 32 instead of 16. | |
39–40 | I guess technically yes, but I think it's fine as-is. So no. |
libcxx/include/experimental/__simd/traits.h | ||
---|---|---|
86 | I'm a bit confuse about this. If no `class = enable_if_t<...>' here, there will be errors coming out. You can see the same situation in D144363#4497271 with 'simd_size'. | |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
29–30 | In that case, how should we modify this? Just add LIBCPP_STATIC_ASSERT temporarily? |
libcxx/include/experimental/__simd/traits.h | ||
---|---|---|
86 | I think we have to do something like template < class _Tp, class _Up = typename _Tp::value_type, bool = (is_simd_v<_Tp> && __is_vectorizable_v<_Up>) || (is_simd_mask_v<_Tp> && is_same_v<_Up, bool>)> struct memory_alignment : integral_constant<size_t, memory_alignment_v<_Tp, _Up>> {}; template <class _Tp, class _Up> struct memory_alignment<Tp, _Up, false> {}; And yeah, this also applies to simd_size. The SFINAE tests you added are correct in that they ensure that there is no value member. I think the enable_if and test for simd_size_v can be removed, since they aren't required. Sorry for not spotting this earlier. |
Thanks! LGTM with comments addressed.
libcxx/include/__bit/bit_ceil.h | ||
---|---|---|
31 ↗ | (On Diff #540628) | I think it should be fine to just remove the _LIBCPP_CONSTEXPR_SINCE_CXX14. It's not like it makes much of a difference in compile times. |
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp | ||
2 | I think you're missing a test that you can instantiate memory_alignment with an invalid type. | |
29–30 | Yeah, I thin a LIBCPP_STATIC_ASSERT is the right call here. |
@philnik Could you please help me to look at the CI errors? I'm not very familiar with these issues. Thank you.
Fix CI
libcxx/include/__bit/bit_ceil.h | ||
---|---|---|
32 ↗ | (On Diff #540939) | Seems we can't use brace-initialization of scalar types in C++03. I tried to fix using: return static_cast<_Tp>(1) << __n; |
@philnik It seems the CI errors related to constexpr for bit_ceil? I'm not sure how to fix. Do you have any solutions?
I think the problem is that you changed the if constexpr to an if. The simplest solution is probably to change it back to an if constexpr and constraining __bit_ceil to _LIBCPP_STD_VER >= 17.
Looks like you forgot to update the status page.