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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added subscribers: Mordante, ldionne, philnik.
philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

A few high-level comments for now. This PR is still quite complex, let's move some of the refactoring stuff into their own PRs.



================
Comment at: libcxx/include/experimental/__simd/scalar.h:16
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI
+
----------------
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)


================
Comment at: libcxx/include/experimental/simd:20
 inline namespace parallelism_v2 {
+  namespace simd_abi {
+    using scalar = see below;
----------------
The synopsis should be updated as we go, to show what has actually been implemented. I am aware that this isn't the case currently, so let's fix this.


================
Comment at: libcxx/include/experimental/simd:39
+
+  // 9.4, simd type traits
+  template<class T> struct is_abi_tag;
----------------
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.


================
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
 
----------------
Why do you remove all of the old implementation? Is it so radically different from what you want to implement?


================
Comment at: libcxx/include/experimental/simd:278
+
+  template<class _Up> __simd_reference operator+=(_Up&& __x) && noexcept;
+  template<class _Up> __simd_reference operator-=(_Up&& __x) && noexcept;
----------------
Could you remove all the declaration-only stuff in a separate PR? We normally don't have the interface declared if it's not defined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144362