[libcxx-commits] [PATCH] D144364: [libcxx] <experimental/simd> Added internal storage type, constructors, subscript operators of class simd/simd_mask and related tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/experimental/__simd/reference.h:20
+template <class _Tp, class _Storage, class _Vp>
+class __reference { // exposition only
+  template <class, class>
----------------
No need to call it out. If it's _Uglified it's library-internal.


================
Comment at: libcxx/include/experimental/__simd/reference.h:20
+template <class _Tp, class _Storage, class _Vp>
+class __reference { // exposition only
+  template <class, class>
----------------
philnik wrote:
> No need to call it out. If it's _Uglified it's library-internal.
`__reference` as a name is really generic.


================
Comment at: libcxx/include/experimental/__simd/reference.h:46
+
+  _LIBCPP_HIDE_FROM_ABI operator value_type() const noexcept { return __get(); }
+};
----------------
It looks like you don't have any tests for the reference type currently. Please add some.


================
Comment at: libcxx/include/experimental/__simd/scalar.h:30-31
+
+  _Tp __get(size_t __idx) const noexcept { return (&__data)[__idx]; }
+  void __set(size_t __idx, _Tp __v) noexcept { (&__data)[__idx] = __v; }
+};
----------------
Why make it so complicated? I'd just go for
```lang=c++
// __get
return __data;

// __set
__data = __v;
```
Also, let's add a few `_LIBCPP_ASSERT_UNCATEGORIZED` (might need a rebase?) to make sure the index is never out of bounds. 


================
Comment at: libcxx/include/experimental/__simd/scalar.h:39-40
+struct __simd_operations<_Tp, simd_abi::__scalar> {
+  using _Simd = __simd_storage<_Tp, simd_abi::__scalar>;
+  using _Mask = __mask_storage<_Tp, simd_abi::__scalar>;
+};
----------------
That makes it a bit more obvious what you do when using it.


================
Comment at: libcxx/include/experimental/__simd/utility.h:45
+  else
+    static_assert(sizeof(_Tp) == 0);
+}
----------------



================
Comment at: libcxx/include/experimental/__simd/utility.h:52-53
+  _Up __res = 0;
+  for (unsigned long __i = 0; __i < __CHAR_BIT__ * sizeof(_Tp); __i++)
+    __res |= static_cast<_Up>(__v) << __i;
+  return __res;
----------------
Isn't this just `__v ? std::numeric_limits<_Up>::max() : 0`?


================
Comment at: libcxx/include/experimental/__simd/vec_ext.h:30-34
+#  if defined(_LIBCPP_COMPILER_CLANG_BASED)
+  _Tp __data __attribute__((vector_size(sizeof(_Tp) * _Np)));
+#  else
+  _Tp __data __attribute__((vector_size(std::__bit_ceil((sizeof(_Tp) * _Np)))));
+#  endif
----------------
Why the conditional? Does this somehow help optimizations?


================
Comment at: libcxx/include/experimental/__simd/vec_ext.h:31
+#  if defined(_LIBCPP_COMPILER_CLANG_BASED)
+  _Tp __data __attribute__((vector_size(sizeof(_Tp) * _Np)));
+#  else
----------------
clang-tidy should actually complain here. I'll have to look into this.


================
Comment at: libcxx/include/experimental/__simd/vec_ext.h:42
+struct __mask_storage<_Tp, simd_abi::__vec_ext<_Np>>
+    : __simd_storage<decltype(std::experimental::parallelism_v2::__choose_mask_type<_Tp>()), simd_abi::__vec_ext<_Np>> {
+};
----------------



================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:53-58
+template <class T>
+struct is_simd_reference : std::false_type {};
+
+template <class T, class S, class V>
+struct is_simd_reference<ex::__reference<T, S, V>> : std::true_type {};
+
----------------
This isn't portable, and I don't really see the point. Nobody cares what the type is named internally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144364