[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 inline comments.


================
Comment at: libcxx/include/experimental/__simd/aligned_tag.h:36
+  static _LIBCPP_HIDE_FROM_ABI constexpr _Up* __apply(_Up* __ptr) {
+    return static_cast<_Up*>(__builtin_assume_aligned(__ptr, __alignment<_Tp, _Up>));
+  }
----------------
Joy12138 wrote:
> philnik wrote:
> > Let's add `__assume_aligned` in `__memory/assume_aligned.h` to avoid using builtins randomly in the code. This lets us also assert if the alignment assumption isn't met (Please add a test for that!).
> If I understand correctly, what you want is a false test when provides incorrect alignment. Then both tests in this patch do not involve alignment inputs. Can we add these false tests in subsequent patches about constructors and load/store that involved with alignment inputs?
Oh yeah, sure. We can test it through the public interface.


================
Comment at: libcxx/include/experimental/__simd/traits.h:80
+    class _Up = typename _Tp::value_type,
+    class = enable_if_t<(is_simd_v<_Tp> && __is_vectorizable<_Up>()) || (is_simd_mask_v<_Tp> && is_same_v<_Up, bool>)>>
+inline constexpr size_t memory_alignment_v = vector_aligned_tag::__alignment<_Tp, _Up>;
----------------
Joy12138 wrote:
> philnik wrote:
> > philnik wrote:
> > > Same here - does this even work with variables and this needs a SFINAE test.
> > Isn't this implicit through `is_simd_mask_v`?
> I don't think so. `_Up` should be able to be provided by the user. `_Tp::value_type` is just a default type.
Ah, I missed that `_Up` may be provided by the user. Never mind then.


================
Comment at: libcxx/include/experimental/__simd/utility.h:27-44
+_LIBCPP_HIDE_FROM_ABI constexpr size_t __bit_ceil(size_t __val) {
+  size_t __pow = 1;
+  while (__pow < __val)
+    __pow <<= 1;
+  return __pow;
+}
+
----------------
Let's add these in their respective implementation detail headers. That way we avoid code duplication and can use them in other places in the codebase.


================
Comment at: libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp:26
+  void operator()() {
+    static_assert(ex::memory_alignment<ex::simd_mask<T, SimdAbi>>{}.value ==
+                  bit_ceil(sizeof(bool) * ex::simd_size_v<T, SimdAbi>));
----------------
Again, to make sure the value is static.


================
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>));
+  }
----------------
It looks to me like this is technically implementation-defined. Am I correct in that interpretation?


================
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(
----------------
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.


================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:54
 #endif // TEST_UTIL_H
\ No newline at end of file

----------------
Newline!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153319