[libcxx-commits] [PATCH] D144362: [libcxx] Updated <experimental/simd> based on Parallelism-TS N4808

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

Please mark comments you have addressed as "Done" to make it easier to see what you have already worked on.



================
Comment at: libcxx/include/experimental/simd:71-75
+    using scalar = __scalar;
+    template<int _Np> using fixed_size = __vec_ext<_Np>;
+    template<class _Tp> inline constexpr int max_fixed_size = 32;
+    template<class _Tp> using compatible = __vec_ext<16 / sizeof(_Tp)>;
+    template<class _Tp> using native = __vec_ext<_LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES / sizeof(_Tp)>;
----------------
I think we should define the aliases in the headers where the actual tags are.


================
Comment at: libcxx/include/experimental/simd:74
+    template<class _Tp> inline constexpr int max_fixed_size = 32;
+    template<class _Tp> using compatible = __vec_ext<16 / sizeof(_Tp)>;
+    template<class _Tp> using native = __vec_ext<_LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES / sizeof(_Tp)>;
----------------
This seems like something that might change from platform to platform. Could you add a `TODO: make this platform dependent`?


================
Comment at: libcxx/include/experimental/simd:85-93
+  // class template simd [simd.class]
+  template<class _Tp, class _Abi = simd_abi::compatible<_Tp>> class simd;
+  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>>;
+
+  // class template simd_mask [simd.mask.class]
+  template<class _Tp, class _Abi = simd_abi::compatible<_Tp>> class simd_mask;
----------------
These should probably get their own headers. I guess the classes are not super small.


================
Comment at: libcxx/include/experimental/simd:96-99
+  template<class _Tp, class = void> struct __is_abi_tag_impl : false_type {};
+  template<class _Tp> struct __is_abi_tag_impl<_Tp, enable_if_t<_Tp::__is_abi_tag>> : bool_constant<_Tp::__is_abi_tag> {};
+  template<class _Tp> struct is_abi_tag : __is_abi_tag_impl<_Tp> {};
+  template<class _Tp> inline constexpr bool is_abi_tag_v = is_abi_tag<_Tp>::value;
----------------
I think it would be easier to move this to it's own header, and then specialize `is_abi_tag` per ABI tag, i.e.
```lang=c++
// traits.h
template <class _Tp>
inline constexpr bool is_abi_tag_v = false;

template <class _Tp>
struct is_abi_tag : bool_constant<is_abi_tag_v<_Tp>> {};

// scalar.h
template <>
inline constexpr bool is_abi_tag_v<__scalar> = true;

// vec_ext.h
template <int _Np>
inline constexpr bool is_abi_tag_v<__vec_ext<_Np>> = true;
```
Note that this also implements `is_abi_tag` in terms of `is_abi_tag_v` and not the other way around. Most people just use the latter, so we should avoid instantiating a class every time. This part also applies to `is_simd` and `is_simd_mask`.


================
Comment at: libcxx/include/experimental/simd:68
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL
+inline namespace parallelism_v2 {
----------------
Joy12138 wrote:
> philnik wrote:
> > Since this is all new code, please clang-format the whole file.
> I tried `git clang-format HEAD~1` and it said `clang-format did not modify any files`.
> 
> Is there anything in this file that does not conform to the LLVM/libcxx Coding Standards?
`git clang-format` doesn't know this file is supposed to be formatted, since it doesn't have a file extension. You can (and should) remove `libcxx/include/experimental/simd` from `libcxx/utils/data/ignore_format.txt`. The CI then shows you what has to be formatted. It should also be possible to do `clang-format libcxx/include/experimental/simd`. That way clang-format knows that it's supposed to format this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144362