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

[Yin Zhang via Phabricator via libcxx-commits]

Joy12138 marked 5 inline comments as done.
Joy12138 added inline comments.


================
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>> {};
----------------
philnik wrote:
> 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.
I'm a bit confuse about this. If no `class = enable_if_t<...>' here, there will be errors coming out. You can see the same situation in D144363#4497271 with 'simd_size'.


================
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>));
+  }
----------------
philnik wrote:
> 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.
In that case, how should we modify this? Just add `LIBCPP_STATIC_ASSERT` temporarily?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153319