[PATCH] D41376: [libcxx] Implement <simd> ABI for Clang/GCC vector extension, constructors, copy_from and copy_to.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Looks good; a bunch of minor things.
Remember to target c++17.
You need to indulge your OCD when writing the tests.



================
Comment at: libcxx/include/__config:1329
 
+#if !defined(_LIBCPP_COMPILER_CLANG) && !defined(_LIBCPP_COMPILER_GCC)
+#define _LIBCPP_HAS_NO_VECTOR_EXTENSION
----------------
We ususally organize these into "chunks", one per compiler.
The "Clang chunk" starts on 159, and goes to 483.
The "GCC chunk" starts on 484 and goes to  575.

It would match the organization better if you put this line:
    #define _LIBCPP_HAS_NO_VECTOR_EXTENSION
in the file twice - once in the Clang chunk and once in the GCC chunk.

See `_LIBCPP_HAS_UNIQUE_OBJECT_REPRESENTATIONS` for an example of this.


================
Comment at: libcxx/include/experimental/simd:726
+#if defined(_LIBCPP_COMPILER_CLANG)
+#define _SPECIALIZE_VEC_EXT(_TYPE, _NUM_ELEMENT)                               \
+  template <>                                                                  \
----------------
When we define user-visible macros in libc++, we prepend them with _LIBCPP_ to avoid collisions.
Accordingly, `_SPECIALIZE_VEC_EXT` should be named `_LIBCPP_SPECIALIZE_VEC_EXT`



================
Comment at: libcxx/include/experimental/simd:936
 
+template <class _Tp>
+struct __nodeduce {
----------------
We got rid of this for N4755 - but I think you're fixing this in D44663.


================
Comment at: libcxx/include/experimental/simd:964
 template <class _Tp>
-using native = compatible<_Tp>;
+using native =
+#ifndef _LIBCPP_HAS_NO_VECTOR_EXTENSION
----------------
I have a weak preference for putting entire statements inside `#ifdef` blocks where possible.


================
Comment at: libcxx/include/experimental/simd:1341
 // [simd.class]
 // TODO: implement simd
 template <class _Tp, class _Abi>
----------------
Is this TODO still necessary?


================
Comment at: libcxx/test/std/experimental/simd/simd.cons/broadcast.pass.cpp:79
+}
+
+int main() { test_broadcast(); }
----------------
Again, do we care about other types than `native_simd` here?


================
Comment at: libcxx/test/std/experimental/simd/simd.cons/default.pass.cpp:21
+using namespace std::experimental::parallelism_v2;
+
+int main() { (void)native_simd<int32_t>(); }
----------------
Do we need any other ctors tested here? `fixed_size_simd<int32_t, 4>` for example?
Are there any post-conditions on the object created?

calling `size()` for example?



================
Comment at: libcxx/test/std/experimental/simd/simd.mem/load.pass.cpp:22
+
+using namespace std::experimental::parallelism_v2;
+
----------------
I am unsure about the desirability of testing this by hoisting it all into the global namespace.

for the pmr stuff, we did:
    namespace ex = std::experimental::pmr;

and then referred to things as `ex::XXX`.  That way, you don't have to do all the typing, but we don't hide any ADL issues.

Obviously, this applies to all the tests.


================
Comment at: libcxx/test/std/experimental/simd/simd.mem/store.pass.cpp:10
+
+// UNSUPPORTED: c++98, c++03
+
----------------
more unsupported :-)


================
Comment at: libcxx/test/std/experimental/simd/simd.mem/store.pass.cpp:51
+
+#if TEST_STD_VER > 14
+  {
----------------
Don't need this anymore.


https://reviews.llvm.org/D41376