[PATCH] D41415: [libcxx] implement <experimental/simd> casts.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.
Herald added subscribers: bixia, jlebar.

In general, this looks good to me.
I suggested a lot of test improvements, but not very much to the code.
Needs the same kind of namespace changes in the tests as in https://reviews.llvm.org/D41376, but that's a minor thing.



================
Comment at: libcxx/include/experimental/simd:1068
   static typename std::enable_if<simd<_Up, _Abi>::size() ==
                                      simd<_Tp, _NewAbi>::size(),
                                  simd<_Tp, _NewAbi>>::type
----------------
Weird indentation in the `enable_if`


================
Comment at: libcxx/test/std/experimental/simd/simd.casts/simd_cast.pass.cpp:19
 #include <cstdint>
 
 using namespace std::experimental::parallelism_v2;
----------------
All the tests should `#include "test_macros.h"`


================
Comment at: libcxx/test/std/experimental/simd/simd.casts/simd_cast.pass.cpp:22
 
+template <class T, class... Args>
+auto unsupported_cast(Args&&... args)
----------------
I think this is worth a comment here - "This conversion is deleted so that it prevents ..."


================
Comment at: libcxx/test/std/experimental/simd/simd.casts/static_simd_cast.pass.cpp:33
 static_assert(
     std::is_same<decltype(static_simd_cast<simd<float, simd_abi::scalar>>(
                      fixed_size_simd<int, 1>())),
----------------
General comment: We have a macro called `ASSERT_SAME_TYPE` that makes this clearer.

Instead of all this, you can just write:
   ASSERT_SAME_TYPE(decltype(static_simd_cast<simd<float, simd_abi::scalar>>(fixed_size_simd<int, 1>())), simd<float, simd_abi::scalar);


================
Comment at: libcxx/test/std/experimental/simd/simd.casts/to_compatible.pass.cpp:39
+
+void test_to_compatible_extension() {
+  auto arr = split_by<32 / simd<int>::size()>(
----------------
Since this is an extension, then the test needs to live in test/libcxx, not test/std.
(and should probably be two tests, one in test/std, and one in test/libcxx).

Same for to_native.pass.cpp below.


================
Comment at: libcxx/test/std/experimental/simd/simd.casts/to_fixed_size.pass.cpp:30
+void test_to_fixed_size() {
+  auto v = to_fixed_size(native_simd<int>([](int i) { return i; }));
+  static_assert(std::is_same<fixed_size_simd<int, native_simd<int>::size()>,
----------------
All of these tests need to check the "noexcept-ness" of the operation.
Fortunately, we have a macro for that, too:

    ASSERT_NOEXCEPT (         to_fixed_size(std::declval<native_simd<int>>()));
    ASSERT_SAME_TYPE(decltype(to_fixed_size(std::declval<native_simd<int>>())), fixed_size_simd<int, native_simd<int>::size()>);



https://reviews.llvm.org/D41415