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

[Tim Shen via Phabricator via cfe-commits]

timshen added inline comments.


================
Comment at: libcxx/include/experimental/simd:726
+#if defined(_LIBCPP_COMPILER_CLANG)
+#define _SPECIALIZE_VEC_EXT(_TYPE, _NUM_ELEMENT)                               \
+  template <>                                                                  \
----------------
mclow.lists wrote:
> 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`
> 
Well _SPECIALIZE_VEC_EXT and _SPECIALIZE_VEC_EXT_32 are not user-visible, as they are (1) '#undef'ed after all uses, and (2) they start with underscore, so don't collide with existing user-defined macros.


================
Comment at: libcxx/include/experimental/simd:1341
 // [simd.class]
 // TODO: implement simd
 template <class _Tp, class _Abi>
----------------
mclow.lists wrote:
> Is this TODO still necessary?
I think so, as some operations are still not implemented, for example operator++().


================
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>(); }
----------------
mclow.lists wrote:
> 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?
> 
Yes. Added more.

The test space is large and it's hard to test everything. I went "every line of the implementation should be covered" standard with myself, and only had native_simd's default ctor. fixed_size_simd<>'s ctor is exactly implemented by the same line, so I didn't bother.

But now the attention is already raised, it doesn't hurt to add more tests.


================
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>(); }
----------------
timshen wrote:
> mclow.lists wrote:
> > 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?
> > 
> Yes. Added more.
> 
> The test space is large and it's hard to test everything. I went "every line of the implementation should be covered" standard with myself, and only had native_simd's default ctor. fixed_size_simd<>'s ctor is exactly implemented by the same line, so I didn't bother.
> 
> But now the attention is already raised, it doesn't hurt to add more tests.
Yes, size() is the post-condition. There is not much beyond that - the elements are not even value initialized.


https://reviews.llvm.org/D41376