[libcxx-commits] [PATCH] D144363: [libcxx] <experimental/simd> Added simd width functions, simd_size traits and related tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

The implementation itself looks good, but the tests need some work.



================
Comment at: libcxx/docs/Status/ParallelismProjects.csv:11
 | `[parallel.simd.traits] <https://wg21.link/N4808>`_, "simd type traits is_simd_flag_type", None, Yin Zhang, |In Progress|
-| `[parallel.simd.traits] <https://wg21.link/N4808>`_, "simd type traits simd_size", None, Yin Zhang, |In Progress|
+| `[parallel.simd.traits] <https://wg21.link/N4808>`_, "`simd type traits simd_size[_v] <https://reviews.llvm.org/D144363>`_", [parallel.simd.traits] is_abi_tag[_v], Yin Zhang, |Complete|
 | `[parallel.simd.traits] <https://wg21.link/N4808>`_, "simd type traits memory_alignment", None, Yin Zhang, |In Progress|
----------------
This looks wrong. Should there have been two lines?


================
Comment at: libcxx/docs/Status/ParallelismProjects.csv:17
 | `[parallel.simd.class] <https://wg21.link/N4808>`_, "`Class template simd declaration and alias <https://reviews.llvm.org/D144362>`_", [parallel.simd.abi], Yin Zhang, |Complete|
+| `[parallel.simd.class] <https://wg21.link/N4808>`_, "`Class template simd width function <https://reviews.llvm.org/D144363>`_", [parallel.simd.traits] simd_size[_v], Yin Zhang, |Complete|
 | `[parallel.simd.class] <https://wg21.link/N4808>`_, "Class template simd implementation", None, Yin Zhang, |In Progress|
----------------
I'm a bit confused by this entry and the one below. What exactly are you trying to mark as completed?


================
Comment at: libcxx/include/experimental/__simd/traits.h:55
+          class _Abi = simd_abi::compatible<_Tp>,
+          class      = enable_if_t<__is_vectorizable<_Tp>() && is_abi_tag_v<_Abi>>>
+inline constexpr size_t simd_size_v = _Abi::__simd_size;
----------------
This needs a SFINAE test. I'm also not sure this works on the `_v` version.


================
Comment at: libcxx/include/experimental/__simd/utility.h:10
 
-#ifndef _LIBCPP_EXPERIMENTAL___SIMD_SIMD_H
-#define _LIBCPP_EXPERIMENTAL___SIMD_SIMD_H
+#ifndef _LIBCPP_EXPERIMENTAL___SIMD_UTILITY_H
+#define _LIBCPP_EXPERIMENTAL___SIMD_UTILITY_H
----------------
`utility` doesn't really seem like that nice of a name for a header. Do you have plans to add more functions? If yes, which ones?


================
Comment at: libcxx/include/experimental/__simd/utility.h:13
 
-#include <experimental/__simd/abi_tag.h>
+#include <type_traits>
 
----------------
Please includes the granularized headers instead.


================
Comment at: libcxx/include/experimental/__simd/utility.h:20
 template <class _Tp>
-using native_simd = simd<_Tp, simd_abi::native<_Tp>>;
-
-template <class _Tp, int _Np>
-using fixed_size_simd = simd<_Tp, simd_abi::fixed_size<_Np>>;
-
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_vectorizable() {
+  return is_arithmetic_v<_Tp> && !is_const_v<_Tp> && !is_volatile_v<_Tp> && !is_same_v<_Tp, bool>;
----------------
Why isn't this simply a variable template?


================
Comment at: libcxx/test/std/experimental/simd/simd.class/simd_width.pass.cpp:23-24
+  void operator()() {
+    const ex::simd<_Tp, SimdAbi> origin_simd{};
+    static_assert(origin_simd.size() == ex::simd_size_v<_Tp, SimdAbi>);
+  }
----------------
This way we make sure the functions is `static` and `constexpr`.


================
Comment at: libcxx/test/std/experimental/simd/simd.class/simd_width.pass.cpp:31
+void test_simd_abi() {
+  F{}.template operator()<_Tp, SimdAbi>();
+  test_simd_abi<F, _Np, _Tp, SimdAbis...>();
----------------
Why aren't you calling `CheckSimdWidth` directly?


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/simd_size.pass.cpp:62
+  void operator()() {
+    static_assert(ex::simd_size<_Tp, SimdAbi>::value == 16 / sizeof(_Tp));
+  };
----------------
The 16 here and `_LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES` are implementation-defined, so this should either be moved into a new test in `test/libcxx` or be a `LIBCPP_STATIC_ASSERT`.


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/simd_size.pass.cpp:85-120
+template <class F, std::size_t _Np, class _Tp>
+void test_simd_abi() {}
+template <class F, std::size_t _Np, class _Tp, class SimdAbi, class... SimdAbis>
+void test_simd_abi() {
+  if constexpr (std::is_same_v<F, CheckSimdSizeScalar> || std::is_same_v<F, CheckSimdSizeVScalar>) {
+    F{}.template operator()<_Tp, ex::simd_abi::scalar>();
+  } else if constexpr (std::is_same_v<F, CheckSimdSizeDeduce> || std::is_same_v<F, CheckSimdSizeVDeduce>) {
----------------
This looks like a really complicated way to do this. For scalar, compatible and native it doesn't look like you make any use of the instantiation facility. They could instead just be in the `main()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144363