Re: [PATCH] std::experimental::simd

[Matthias Kretz <m.kretz@gsi.de>]

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
──────────────────────────────────────────────────────────────────────────