[libcxx-commits] [PATCH] D156225: [libcxx] <experimental/simd> Add broadcast constructor of class simd/simd_mask

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/experimental/__simd/utility.h:71
+    (!__is_vectorizable_v<_Up> && is_convertible_v<_Up, _Tp>) || is_same_v<_Up, int> ||
+    (is_same_v<_Up, unsigned int> && is_unsigned_v<_Tp>);
+
----------------
Joy12138 wrote:
> philnik wrote:
> > I'm pretty sure that `unsigned int` is indeed unsigned.
> I'm not sure, but have you confused `_Up` and `_Tp`?
Oops. Yes, I missed that there is _Tp and _Up. Just ignore the comment.


================
Comment at: libcxx/include/experimental/__simd/utility.h:60-65
+template <typename __From, typename __To, typename = void>
+struct __is_value_preserving_convertible : std::false_type {};
+
+template <typename __From, typename __To>
+struct __is_value_preserving_convertible<__From, __To, decltype(static_cast<__To>(std::declval<__From>()))>
+    : std::true_type {};
----------------
Joy12138 wrote:
> philnik wrote:
> > Joy12138 wrote:
> > > philnik wrote:
> > > > Isn't this just `is_constructible<__To, __From>`?
> > > I'm not sure if the current implementation is the same as `is_constructible`.
> > > I am also not sure if either of them can meet the requirements of the document: "every possible value of `From` can be represented with type `To`".
> > > Do you have any suggestion?
> > I think we have to do something a bit more weird for that. I think we basically have to check that they are integral and that `sizeof(From) >= sizeof(To) && is_signed_v<From> == is_signed_v<To>`. I'm  not sure about floating point types, but there we probably want to do the same % `bfloat16` (but that's not relevant for now anyways).
> > 
> > Also, we probably want to make these variable templates instead of structs. Didn't notice that before.
> According to your description, I think what you want is to check for non narrowing conversions. I have learned that brace initialization might ensure non narrowing conversions. What do you think about current implementation?
Good thought! This should indeed do the right thing. You're only missing `inline`s to avoid ODR violations.


================
Comment at: libcxx/test/std/experimental/simd/simd.class/simd_ctor_broadcast.pass.cpp:61-77
+    constexpr std::size_t array_size = ex::simd_size_v<T, SimdAbi>;
+    std::array<T, array_size> expected_value;
+    for (std::size_t i = 0; i < array_size; ++i)
+      expected_value[i] = static_cast<T>(3);
+
+    implicit_type<T> implicit_convert_to_3(3);
+    ex::simd<T, SimdAbi> simd_broadcast_from_implicit_type(std::move(implicit_convert_to_3));
----------------
Or am I missing something?


================
Comment at: libcxx/test/std/experimental/simd/simd.class/simd_ctor_broadcast.verify.cpp:30
+  ex::native_simd<int> s1(2.0f);
+  // expected-error-re@* {{no matching constructor for initialization of {{.*}}}}
+
----------------
These tests should be SFINAE checks instead, since the function shall not participate in overload resolution. This construct is not always ill-formed.


================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:62
+template <std::size_t ArraySize, class SimdAbi, class T, class U = T>
+void assert_simd_value_correct(const ex::simd<T, SimdAbi>& origin_simd,
+                               const std::array<U, ArraySize>& expected_value) {
----------------



================
Comment at: libcxx/test/std/experimental/simd/test_utils.h:69
+template <std::size_t ArraySize, class T, class SimdAbi>
+void assert_simd_mask_value_correct(const ex::simd_mask<T, SimdAbi>& origin_mask,
+                                    const std::array<bool, ArraySize>& expected_value) {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156225