[libcxx-commits] [PATCH] D144364: [libcxx] <experimental/simd> Added internal storage type, constructors, subscript operators of class simd/simd_mask and related tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/experimental/__simd/vec_ext.h:30-34
+#  if defined(_LIBCPP_COMPILER_CLANG_BASED)
+  _Tp __data __attribute__((vector_size(sizeof(_Tp) * _Np)));
+#  else
+  _Tp __data __attribute__((vector_size(std::__bit_ceil((sizeof(_Tp) * _Np)))));
+#  endif
----------------
Joy12138 wrote:
> philnik wrote:
> > Why the conditional? Does this somehow help optimizations?
> `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.
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.


================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:53-58
+template <class T>
+struct is_simd_reference : std::false_type {};
+
+template <class T, class S, class V>
+struct is_simd_reference<ex::__reference<T, S, V>> : std::true_type {};
+
----------------
Joy12138 wrote:
> philnik wrote:
> > This isn't portable, and I don't really see the point. Nobody cares what the type is named internally.
> Then the type check of reference in the `simd[_mask]_alias.pass.cpp` is also unnecessary, right? Can I directly remove them?
> 
Yes, they are unnecessary. You should add tests that check the properties of `simd::reference` and `simd_mask::reference` though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144364/new/

https://reviews.llvm.org/D144364