On Donnerstag, 12. November 2020 00:43:31 CET Jonathan Wakely wrote:
> 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
Done.
> >+ * 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.
Ah good. I'll delete some code.
> >+// 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.
I need those to help cutting down the code for 80 cols. ;-)
> >+// __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?
With GCC it would suffice to use __type_identity<T>::type here. But Clang
rejects it. Clang sees that the first template argument is not used in the
definition of the alias and thus doesn't make _Up a dependent type.
> >+// __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.
These are all [[gnu::always_inline]]. So it shouldn't matter.
> >+// __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.
Right. That was me trying to work around a clang-format bug. Will fix. I'm in
the process of ditching clang-format anyway.
> >+{
> >+};
> >+
> >+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.
Odd indeed. Either I wanted to take a shortcut to implement: "From is a
vectorizable type and every possibly value of From can be represented with
type value_type, or [...]". Or I wanted to swap bool and _Tp here and say that
anything other than bool converting to bool is narrowing.
I should clean this up.
>
> >+// _BitOps {{{
> >+struct _BitOps
> > [...]
> std::__popcount in <bit>
> > [...]
> std::__countl_zero in <bit>
Yes. I'll clean up all of _BitOps.
> >+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.
I believe I checked the list. ;-)
> >+
> >+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.
Called either with integers, [[gnu::vector_size(N)]] types, or
std::experimental::parallelism_v2::_SimdWrapper. I request a column limit
relaxation to at least 100 cols if I should qualify all of them with
std::experimental:: ;-)
> 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.
Thanks. I'll post a new patch ASAP. My tests are running.
-Matthias
--
──────────────────────────────────────────────────────────────────────────
Dr. Matthias Kretz https://mattkretz.github.io
GSI Helmholtz Centre for Heavy Ion Research https://gsi.de
std::experimental::simd https://github.com/VcDevel/std-simd
──────────────────────────────────────────────────────────────────────────
