[PATCH] D41148: [libcxx] implement <experimental/simd> declarations based on P0214R7.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I'm going to stop here, because all the things I've noted are ticky-tack; formatting and minor changes.
More substantial comments coming soon.



================
Comment at: libcxx/include/experimental/simd:40
+template <class T> inline constexpr bool is_abi_tag_v = is_abi_tag<T>::value;
+template <class T> struct is_simd;
+template <class T> inline constexpr bool is_simd_v = is_simd<T>::value;
----------------
A blank line between the separate groups (as in the paper) will make this much easier to read


================
Comment at: libcxx/include/experimental/simd:67
+template <class T, class U, class Abi> see below simd_cast(const simd<U, Abi>&);
+
+template <class T, class U, class Abi> see below static_simd_cast(const simd<U, Abi>&);
----------------
And here you have an extra line - go figure.


================
Comment at: libcxx/include/experimental/simd:113
+template <class T, class Abi> int find_last_set(const simd_mask<T, Abi>&);
+bool all_of(see below ) noexcept;
+bool any_of(see below ) noexcept;
----------------
blank line, extra space `(see below )` should be `(see below)`


================
Comment at: libcxx/include/experimental/simd:144
+const_where_expression<simd_mask<T, Abi>, const simd_mask<T, Abi>>
+where(const nodeduce_t<simd_mask<T, Abi>>&, const simd_mask<T, Abi>&) noexcept;
+
----------------
This is better formatting than in the paper :-)


================
Comment at: libcxx/include/experimental/simd:316
+template <class T, class V> using samesize = fixed_size_simd<T, V::size()>; // exposition only
+template <class Abi> floatv<Abi> acos(floatv<Abi> x);
+template <class Abi> doublev<Abi> acos(doublev<Abi> x);
----------------
These would be much easier to read lined up, and with blank lines.

```
template <class Abi> floatv<Abi>   acos(floatv<Abi>   x);
template <class Abi> doublev<Abi>  acos(doublev<Abi>  x);
template <class Abi> ldoublev<Abi> acos(ldoublev<Abi> x);

template <class Abi> floatv<Abi>   asin(floatv<Abi>   x);
template <class Abi> doublev<Abi>  asin(doublev<Abi>  x);
template <class Abi> ldoublev<Abi> asin(ldoublev<Abi> x);

```



================
Comment at: libcxx/include/experimental/simd:626
+constexpr auto __is_non_narrowing_convertible_impl(_From a [[gnu::unused]])
+    -> decltype(_To{a}, true) {
+  return true;
----------------
Do you need the trailing return type here? instead of something like `decltype(_to{declval<_From>()})`

Then you don't need to name `a`, and can get rid of the (gcc-specific) attribute.


================
Comment at: libcxx/include/experimental/simd:653
+constexpr _Tp __variadic_sum() {
+  return {};
+}
----------------
This won't work if `_Tp` has an explicit default ctor (which is probably not true for anything in SIMD).  Do you lose anything by writing `return _Tp{}` ?



================
Comment at: libcxx/include/experimental/simd:669
+
+#if _LIBCPP_STD_VER > 14 && !defined(_LIBCPP_HAS_NO_VARIABLE_TEMPLATES)
+template <class _Tp>
----------------
Isn't the parallelism TS based on C++17?



https://reviews.llvm.org/D41148