[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 accepted this revision.
philnik added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM with comments addressed.



================
Comment at: libcxx/include/__bit/bit_ceil.h:31
 
-  if constexpr (sizeof(_Tp) >= sizeof(unsigned))
-    return _Tp{1} << __n;
+  if _LIBCPP_CONSTEXPR_SINCE_CXX14 (sizeof(_Tp) >= sizeof(unsigned))
+    return (_Tp){1} << __n;
----------------
I think it should be fine to just remove the `_LIBCPP_CONSTEXPR_SINCE_CXX14`. It's not like it makes much of a difference in compile times.


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think you're missing a test that you can instantiate `memory_alignment` with an invalid type.


================
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:
> > 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?
Yeah, I thin a `LIBCPP_STATIC_ASSERT` is the right call here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153319