[libcxx-commits] [PATCH] D153319: [libcxx] <experimental/simd> Added aliagned flag types, traits is_simd_flag_type[_v], memory_alignment[_v] and related tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

Please rebase. It looks like the CI didn't run properly.



================
Comment at: libcxx/docs/Status/ParallelismProjects.csv:7
 | `[parallel.simd.abi] <https://wg21.link/N4808>`_, "`simd ABI tags <https://reviews.llvm.org/D144362>`_", [parallel.simd.class] [parallel.simd.mask.class] declarations and alias, Yin Zhang, |Complete|
+| `[parallel.simd.aligned] <https://wg21.link/N4808>`_, "`simd aligned tags <https://reviews.llvm.org/D153319>`_", [parallel.simd.class] [parallel.simd.mask.class] declarations, Yin Zhang, |Complete|
 | `[parallel.simd.traits] <https://wg21.link/N4808>`_, "`simd type traits is_abi_tag[_v] <https://reviews.llvm.org/D144362>`_", [parallel.simd.abi], Yin Zhang, |Complete|
----------------
There doesn't seem to be a `[parallel.simd.aligned]` section. AFAICT this is just `[parallel.simd.synopsis]`.


================
Comment at: libcxx/include/__bit/bit_ceil.h:25
+template <class _Tp>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Tp __bit_ceil(_Tp __t) _NOEXCEPT {
   if (__t < 2)
----------------
Otherwise you're lying.


================
Comment at: libcxx/include/__memory/assume_aligned.h:26
 template <size_t _Np, class _Tp>
-[[nodiscard]]
-_LIBCPP_HIDE_FROM_ABI
-constexpr _Tp* assume_aligned(_Tp* __ptr) {
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Tp* __assume_aligned(_Tp* __ptr) {
   static_assert(_Np != 0 && (_Np & (_Np - 1)) == 0,
----------------
This will break C++03 otherwise.


================
Comment at: libcxx/include/experimental/__simd/aligned_tag.h:34
+  template <class _Tp, class _Up = typename _Tp::value_type>
+  static constexpr size_t __alignment = __bit_ceil(sizeof(_Up) * _Tp::size());
+
----------------



================
Comment at: libcxx/include/experimental/__simd/traits.h:88
+    class _Up = typename _Tp::value_type,
+    class = enable_if_t<(is_simd_v<_Tp> && __is_vectorizable_v<_Up>) || (is_simd_mask_v<_Tp> && is_same_v<_Up, bool>)>>
+struct memory_alignment : integral_constant<size_t, memory_alignment_v<_Tp, _Up>> {};
----------------
I think this might actually be wrong. The wording says that `memory_alignment` shouldn't contain `value`, not that the class should fail to instantiate.


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp:28-29
+                  bit_ceil(sizeof(bool) * ex::simd_size_v<T, SimdAbi>));
+    static_assert(
+        ex::memory_alignment_v<ex::simd_mask<T, SimdAbi>> == bit_ceil(sizeof(bool) * ex::simd_size_v<T, SimdAbi>));
+  }
----------------
Joy12138 wrote:
> philnik wrote:
> > It looks to me like this is technically implementation-defined. Am I correct in that interpretation?
> If my understanding of the document is correct, the basic type of the `simd_mask` is `bool`, while the storage type in the internal implementation layer is implementation-defined. Alignment should be calculated using `bool`.
I can't find any wording that suggests a required alignment. Consider a platform with a 32byte vector. If the platform can load that vector from a 16byte aligned address without any loss of performance, there is no reason to make `memory_alignment` return 32 instead of 16.


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp:38-39
+    if constexpr (std::is_same_v<T, long double>) {
+      // The size of long double is platform-specific. For instance, on i686-w64-mingw32-clang++,
+      // the size of long double is 12 bytes. Hence, disambiguation is needed.
+      static_assert(
----------------
Joy12138 wrote:
> philnik wrote:
> > I think this is technically also the case for `float` and `double`. It just happens to be the case that they are the IEEE types on about every platform.
> is there any other modify required here?
I guess technically yes, but I think it's fine as-is. So no.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153319