On 08/05/20 21:03 +0200, Matthias Kretz wrote:
Here's my last update to the std::experimental::simd patch. It's currently
based on the gcc-10 branch.


+
+// __next_power_of_2{{{
+/**
+ * \internal

We use @foo for Doxygen commens rather than \foo

+ * Returns the next power of 2 larger than or equal to \p __x.
+ */
+constexpr std::size_t
+__next_power_of_2(std::size_t __x)
+{
+  return (__x & (__x - 1)) == 0 ? __x
+                               : __next_power_of_2((__x | (__x >> 1)) + 1);
+}

Can this be replaced with std::__bit_ceil ?

std::bit_ceil is C++20, but we provide __private versions of
everything in <bit> for C++14 and up.

+// vvv ---- type traits ---- vvv
+// integer type aliases{{{
+using _UChar = unsigned char;
+using _SChar = signed char;
+using _UShort = unsigned short;
+using _UInt = unsigned int;
+using _ULong = unsigned long;
+using _ULLong = unsigned long long;
+using _LLong = long long;

I have a suspicion some of these might clash with libc macros on some
OS somewhere, but we can cross that bridge when we come to it.


+//}}}
+// __identity/__id{{{
+template <typename _Tp> struct __identity
+{
+  using type = _Tp;
+};
+template <typename _Tp> using __id = typename __identity<_Tp>::type;

<type_traits> provides __type_identity and __type_identity_t.

+
+// }}}
+// __first_of_pack{{{
+template <typename _T0, typename...> struct __first_of_pack
+{
+  using type = _T0;
+};
+template <typename... _Ts>
+using __first_of_pack_t = typename __first_of_pack<_Ts...>::type;

<tr2/type_traits> has __reflection_type_list<T...>::first::type for
this purpose, but nobody uses that header. This is fine.


+//}}}
+// __value_type_or_identity_t {{{
+template <typename _Tp>
+typename _Tp::value_type
+__value_type_or_identity_impl(int);
+template <typename _Tp>
+_Tp
+__value_type_or_identity_impl(float);
+template <typename _Tp>
+using __value_type_or_identity_t
+  = decltype(__value_type_or_identity_impl<_Tp>(int()));

This could be __detected_or_t<_Tp, __value_t, _Tp> but your version
probably compiles faster.


+// }}}
+// __is_vectorizable {{{
+template <typename _Tp>
+struct __is_vectorizable : public std::is_arithmetic<_Tp>
+{
+};
+template <> struct __is_vectorizable<bool> : public false_type
+{
+};
+template <typename _Tp>
+inline constexpr bool __is_vectorizable_v = __is_vectorizable<_Tp>::value;

Specializing __is_vectorizable_v<bool> = false would save needing to
instantiate __is_vectorizable<bool>, but not a big deal.

+// __make_dependent_t {{{
+template <typename, typename _Up> struct __make_dependent
+{
+  using type = _Up;
+};
+template <typename _Tp, typename _Up>
+using __make_dependent_t = typename __make_dependent<_Tp, _Up>::type;

Do you need a distinct class template for this, or can
__make_dependent_t be an alias to __type_identity<T>::type or
something else that already exists?

+// __call_with_n_evaluations{{{
+template <size_t... _I, typename _F0, typename _FArgs>
+_GLIBCXX_SIMD_INTRINSIC constexpr auto
+__call_with_n_evaluations(std::index_sequence<_I...>, _F0&& __f0,
+                         _FArgs&& __fargs)

I'm not sure if it matters here, but old versions of G++ passed empty
types (like index_sequence) using the wrong ABI. Passing them as the
last argument makes it a non-issue. If they're not the last argument,
you get incompatible code when compiling with -fabi-version=7 or
lower.

+// __is_narrowing_conversion<_From, _To>{{{
+template <typename _From, typename _To, bool = 
std::is_arithmetic<_From>::value,
+         bool = std::is_arithmetic<_To>::value>

These could use is_arithmetic_v.

+struct __is_narrowing_conversion;
+
+// ignore "warning C4018: '<': signed/unsigned mismatch" in the following 
trait.
+// The implicit conversions will do the right thing here.
+template <typename _From, typename _To>
+struct __is_narrowing_conversion<_From, _To, true, true>
+  : public __bool_constant<(
+      std::numeric_limits<_From>::digits > std::numeric_limits<_To>::digits
+      || std::numeric_limits<_From>::max() > std::numeric_limits<_To>::max()
+      || std::numeric_limits<_From>::lowest()
+          < std::numeric_limits<_To>::lowest()
+      || (std::is_signed<_From>::value && std::is_unsigned<_To>::value))>

And is_signed_v and is_unsigned_v.

+{
+};
+
+template <typename _Tp>
+struct __is_narrowing_conversion<bool, _Tp, true, true> : public true_type

This looks odd, bool to arithmetic type T is narrowing?
I assume there's a reason for it, so maybe a comment explaining it
would help.

+// _BitOps {{{
+struct _BitOps
+{
+  // __popcount {{{
+  static constexpr _UInt __popcount(_UInt __x)
+  {
+    return __builtin_popcount(__x);
+  }
+  static constexpr _ULong __popcount(_ULong __x)
+  {
+    return __builtin_popcountl(__x);
+  }
+  static constexpr _ULLong __popcount(_ULLong __x)
+  {
+    return __builtin_popcountll(__x);
+  }

std::__popcount in <bit>

+  // }}}
+  // __ctz/__clz {{{
+  static constexpr _UInt __ctz(_UInt __x) { return __builtin_ctz(__x); }
+  static constexpr _ULong __ctz(_ULong __x) { return __builtin_ctzl(__x); }
+  static constexpr _ULLong __ctz(_ULLong __x) { return __builtin_ctzll(__x); }
+  static constexpr _UInt __clz(_UInt __x) { return __builtin_clz(__x); }
+  static constexpr _ULong __clz(_ULong __x) { return __builtin_clzl(__x); }
+  static constexpr _ULLong __clz(_ULLong __x) { return __builtin_clzll(__x); }

std::__countl_zero in <bit>

+template <typename _V>

We generally avoid single letter names, although _V isn't in the list
of BADNAMES in the manual, so maybe this one's OK.

+template <typename _Tp, typename _TVT = _VectorTraits<_Tp>,
+         typename _R

Same for _R, it's not listed as a BADNAME.

+
+template <typename _Tp, typename = decltype(_Tp() & _Tp())>
+_GLIBCXX_SIMD_INTRINSIC constexpr _Tp
+__and(_Tp __a, _Tp __b) noexcept

Calls to __and are done unqualified. Are they only with types that
won't cause ADL to look outside namespace std?

Even though __and is a reserved name, avoidign ADL has other benefits.


That's all for now ... not very far through the huge patch though.
Generally this looks very good. The things mentioned above are
stylistic or just remove some redundancy, they're not critical.


Reply via email to