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

[Yin Zhang via Phabricator via libcxx-commits]

Joy12138 marked an inline comment as done.
Joy12138 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>);
+
----------------
philnik wrote:
> I'm pretty sure that `unsigned int` is indeed unsigned.
I'm not sure, but have you confused `_Up` and `_Tp`?


================
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 {};
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156225