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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this. Is N4808 voted in? I can only find a draft version.



================
Comment at: libcxx/include/experimental/__simd/scalar.h:16
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI
+
----------------
philnik wrote:
> IMO it would be a lot nicer to just use
> ```lang=c++
> namespace std::experimental {
> inline namespace parallelism_v2 {
> 
> } // namespace parallelism_v2
> } // namespace std::experimental
> ```
> While it's marginally more verbose, it's a lot easier to read. (`namespace std::experimental::inline parallelism_v2` doesn't work because that's C++20)
> @ldionne @Mordante any thoughts here?
> 
> (Don't apply this before others have commented, this is a more general discussion)
I don't have a real preference. IIRC `_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI` is the more typical way in libc++.


================
Comment at: libcxx/include/experimental/simd:39
+
+  // 9.4, simd type traits
+  template<class T> struct is_abi_tag;
----------------
philnik wrote:
> Let's keep the stable names. Section numbers are just a nightmare to maintain and will be wrong. At least they are everywhere they are used currently.
+1 Especially since these numbers will change if this is adopted on the Standard. Using the stable names gives a higher probability these references remain valid.


================
Comment at: libcxx/include/experimental/simd:248
 
-enum class _StorageKind {
-  _Scalar,
-  _Array,
-  _VecExt,
-};
-
-template <_StorageKind __kind, int _Np>
-struct __simd_abi {};
-
-template <class _Tp, class _Abi>
-class __simd_storage {};
-
-template <class _Tp, int __num_element>
-class __simd_storage<_Tp, __simd_abi<_StorageKind::_Array, __num_element>> {
-  std::array<_Tp, __num_element> __storage_;
-
-  template <class, class>
-  friend struct simd;
-
-  template <class, class>
-  friend struct simd_mask;
-
-public:
-  _Tp __get(size_t __index) const noexcept { return __storage_[__index]; }
-  void __set(size_t __index, _Tp __val) noexcept {
-    __storage_[__index] = __val;
-  }
-};
-
-template <class _Tp>
-class __simd_storage<_Tp, __simd_abi<_StorageKind::_Scalar, 1>> {
-  _Tp __storage_;
-
-  template <class, class>
-  friend struct simd;
-
-  template <class, class>
-  friend struct simd_mask;
-
-public:
-  _Tp __get(size_t __index) const noexcept { return (&__storage_)[__index]; }
-  void __set(size_t __index, _Tp __val) noexcept {
-    (&__storage_)[__index] = __val;
-  }
-};
-
-#ifndef _LIBCPP_HAS_NO_VECTOR_EXTENSION
-
-_LIBCPP_HIDE_FROM_ABI constexpr size_t __floor_pow_of_2(size_t __val) {
-  return ((__val - 1) & __val) == 0 ? __val
-                                    : __floor_pow_of_2((__val - 1) & __val);
-}
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI
 
----------------
philnik wrote:
> Why do you remove all of the old implementation? Is it so radically different from what you want to implement?
I'm wondering the same. Normally we don't remove code, since it may break existing users.
However I think our current implementation is so minimal that risk isn't too great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144362