[libcxx-commits] [PATCH] D153319: [libcxx] <experimental/simd> Added aliagned flag types, traits is_simd_flag_type[_v], memory_alignment[_v] and related tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:882
   deque
   errno.h
   exception
----------------
Looks like you forgot to update the status page.


================
Comment at: libcxx/include/experimental/__simd/aligned_tag.h:36
+  static _LIBCPP_HIDE_FROM_ABI constexpr _Up* __apply(_Up* __ptr) {
+    return static_cast<_Up*>(__builtin_assume_aligned(__ptr, __alignment<_Tp, _Up>));
+  }
----------------
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!).


================
Comment at: libcxx/include/experimental/__simd/traits.h:80
+    class _Up = typename _Tp::value_type,
+    class = enable_if_t<(is_simd_v<_Tp> && __is_vectorizable<_Up>()) || (is_simd_mask_v<_Tp> && is_same_v<_Up, bool>)>>
+inline constexpr size_t memory_alignment_v = vector_aligned_tag::__alignment<_Tp, _Up>;
----------------
Same here - does this even work with variables and this needs a SFINAE test.


================
Comment at: libcxx/include/experimental/__simd/traits.h:80
+    class _Up = typename _Tp::value_type,
+    class = enable_if_t<(is_simd_v<_Tp> && __is_vectorizable<_Up>()) || (is_simd_mask_v<_Tp> && is_same_v<_Up, bool>)>>
+inline constexpr size_t memory_alignment_v = vector_aligned_tag::__alignment<_Tp, _Up>;
----------------
philnik wrote:
> Same here - does this even work with variables and this needs a SFINAE test.
Isn't this implicit through `is_simd_mask_v`?


================
Comment at: libcxx/include/experimental/__simd/utility.h:24-30
+// TODO: replace it by std::bit_ceil when bump to C++20
+_LIBCPP_HIDE_FROM_ABI constexpr size_t __next_pow_of_2(size_t __val) {
+  size_t __pow = 1;
+  while (__pow < __val)
+    __pow <<= 1;
+  return __pow;
+}
----------------
let's instead add a `__bit_ceil` for pre-C++20 usage.


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp:26
+  void operator()() {
+    static_assert(ex::memory_alignment<ex::simd<_Tp, SimdAbi>>{}.value == _LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES);
+    static_assert(ex::memory_alignment_v<ex::simd<_Tp, SimdAbi>> == _LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES);
----------------
Also the comment here - `_LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES` is libc++-specific, so it shouldn't be used in a `std`test.


================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:22
 
+// TODO: replace it by std::bit_ceil when bump to C++20
+constexpr size_t next_pow2(size_t val) {
----------------
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.


================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:23
+// TODO: replace it by std::bit_ceil when bump to C++20
+constexpr size_t next_pow2(size_t val) {
+  size_t pow = 1;
----------------
Since you already mentioned that they are equivalent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153319