public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jonathan Wakely <jwakely@redhat.com>
To: Matthias Kretz <m.kretz@gsi.de>
Cc: Thomas Rodgers <trodgers@redhat.com>,
	libstdc++ <libstdc++@gcc.gnu.org>,
	Gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] std::experimental::simd
Date: Wed, 11 Nov 2020 23:43:31 +0000	[thread overview]
Message-ID: <20201111234331.GA503596@redhat.com> (raw)
In-Reply-To: <33105491.xCRyjBS7g1@excalibur>

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	other threads:[~2020-11-11 23:43 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-14 12:12 Matthias Kretz
2019-10-15  3:52 ` Thomas Rodgers
2019-10-24  8:26 ` Dr. Matthias Kretz
2020-02-10 16:49   ` Thomas Rodgers
2020-02-10 20:14     ` Thomas Rodgers
2020-01-07 11:01 ` Matthias Kretz
2020-01-07 11:17   ` Andrew Pinski
2020-01-07 13:19     ` Dr. Matthias Kretz
     [not found] ` <3486545.znU0eCzeS4@excalibur>
     [not found]   ` <xkqeo8qyl8y8.fsf@trodgers.remote>
2020-05-08 19:03     ` Matthias Kretz
2020-11-11 23:43       ` Jonathan Wakely [this message]
2020-11-14  1:11         ` Matthias Kretz
2020-11-15 19:11           ` Matthias Kretz
2020-12-10 21:13             ` Matthias Kretz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201111234331.GA503596@redhat.com \
    --to=jwakely@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=libstdc++@gcc.gnu.org \
    --cc=m.kretz@gsi.de \
    --cc=trodgers@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).