[libcxx] <experimental/simd> Added internal storage type for class simd/simd_mask
[libcxx] <experimental/simd> Added all constructors of class simd/simd_mask and related tests
[libcxx] <experimental/simd> Added basic simd reference implementation, subscript operators of class simd/simd_mask and related tests
Details
- Reviewers
philnik - Group Reviewers
Restricted Project - Commits
- rGe7a45c6d768b: [libcxx] <experimental/simd> Added internal storage type, constructors…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I mainly look quickly over the patch, can you get the patch to apply properly so the CI can test it?
libcxx/include/experimental/__simd/utility.h | ||
---|---|---|
48 | Will the compiler optimize this? |
Rebase.
Due to the large amount of content in this section, I will split them into two patches to submit.
In this patch, we mainly implements the internal storage type of the simd/simd_mask class and the basic part of its auxiliary class reference.
In the next patch we will implement relevant user interfaces, including constructors and subscript access operators, as well as related tests.
The status paper will be updated in the next patch.
libcxx/include/experimental/__simd/reference.h | ||
---|---|---|
21 | No need to call it out. If it's _Uglified it's library-internal. | |
21 | __reference as a name is really generic. | |
47 | It looks like you don't have any tests for the reference type currently. Please add some. | |
libcxx/include/experimental/__simd/scalar.h | ||
30–31 | Why make it so complicated? I'd just go for // __get return __data; // __set __data = __v; Also, let's add a few _LIBCPP_ASSERT_UNCATEGORIZED (might need a rebase?) to make sure the index is never out of bounds. | |
39–40 | That makes it a bit more obvious what you do when using it. | |
libcxx/include/experimental/__simd/utility.h | ||
45 | ||
52–53 | Isn't this just __v ? std::numeric_limits<_Up>::max() : 0? | |
libcxx/include/experimental/__simd/vec_ext.h | ||
30–34 | Why the conditional? Does this somehow help optimizations? | |
31 | clang-tidy should actually complain here. I'll have to look into this. | |
42 | ||
libcxx/test/std/experimental/simd/test_utils.h | ||
53–58 | This isn't portable, and I don't really see the point. Nobody cares what the type is named internally. |
libcxx/include/experimental/__simd/vec_ext.h | ||
---|---|---|
30–34 | libcxx CI - GCC 12 / C++latest gives errors with vector size that is not a power of two: /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-8484195cd0ed-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1/experimental/__simd/vec_ext.h:30:7: error: number of vector components 3 not a power of two 30 | _Tp __data __attribute__((vector_size(sizeof(_Tp) * _Np))); | ^~~~~~ It seems GCC 12 doesn't support non-power-of-two vector lengths. So I added the conditional. | |
libcxx/test/std/experimental/simd/test_utils.h | ||
53–58 | Then the type check of reference in the simd[_mask]_alias.pass.cpp is also unnecessary, right? Can I directly remove them? |
libcxx/include/experimental/__simd/vec_ext.h | ||
---|---|---|
30–34 | I would just always make it a power of two then. If there is no improvement when introducing the conditional, we can just remove it and make both compilers happy. | |
libcxx/test/std/experimental/simd/test_utils.h | ||
53–58 | Yes, they are unnecessary. You should add tests that check the properties of simd::reference and simd_mask::reference though. |
LGTM with the remaining comments addressed.
libcxx/include/experimental/__simd/reference.h | ||
---|---|---|
34 | To avoid ADL you don't have to fully qualify the name. | |
libcxx/include/experimental/__simd/scalar.h | ||
31 | Note: Once the data is actually reachable, this should get a test. | |
libcxx/include/experimental/__simd/utility.h | ||
50 |
It seems that the long double is a type with a size of 12 bytes on MinGW (DLL, i686). This conflicts with the current implementation. Vector extension requires the input parameter of vector size to be a power of 2 and a multiple of the size of the element type. 12 bytes cannot meet both requirements simultaneously. Should we temporarily exclude long double on MinGW (DLL, i686)? I think it would be more appropriate to add support for it when implement platform specialization optimization. @philnik
No need to call it out. If it's _Uglified it's library-internal.