From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 6F6FE3858D1E for ; Tue, 20 Jun 2023 08:29:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6F6FE3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1687249779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=VVI48Cuv8G8omdLWwGH9BxSNt7DeD8HNbYvSS1DVeHc=; b=NciB5dmz6udKqPedxUd6x3wL5FMSY45Hq/qStKLaniYYRvGC4bWT5qFObc8bzz8+YwvVhc +1LaRXDeD2xhMHY24qzjAkHQ0/rN0zUDyGrm5alT8fe9HjfMU2bqghO0155xh/8mY0lASA MrYWGYeuV82QL1X7IWnKQjx6WTtsLok= Received: from mail-lf1-f69.google.com (mail-lf1-f69.google.com [209.85.167.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-613-uB6-SXuAM1i8yC4zjY88Ow-1; Tue, 20 Jun 2023 04:29:37 -0400 X-MC-Unique: uB6-SXuAM1i8yC4zjY88Ow-1 Received: by mail-lf1-f69.google.com with SMTP id 2adb3069b0e04-4f0176dcc1eso2009186e87.1 for ; Tue, 20 Jun 2023 01:29:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687249776; x=1689841776; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=rM8W8D51PXE3/zdCsgfjEjrIn4yE1ytaOsD0m6oJiks=; b=NDoO4qcby6wbI42AJUzP5T3H5CHirUmFP3UqNdRb8AvgM5yXzbIYxvcnnYBy86oR/F S4d+x/QOCI8GlL1/fntf4nxLjkH3LgMoI+3smN7WOMRH5HpN7464we8qcSZz96E+N0XG yMkJSQiiBHSKxGKiE0BFT4ZbwgLcWsKgb2sX+Q2f+6ga0iwyVXNgUadeuJp/fS/nl/Pu k86+14/0PlqF1gZixRqBGeHemAbtWYKptLCzu127E38Ha/RPel1fUL/EWA6JE6u5V4B0 na+BnIYzno8bCp/pFMY8HEgAJ2kkf5y9X4fvCUaFceRd+MG3764CaIBZd4PqejL6oX+h LFUA== X-Gm-Message-State: AC+VfDxw1tgJf+6O31Mglzgf6Nej6R35g5NoltuPExZHYepcJXYGl/ca QqDbcF6u70NIcFWlyohXryFAtc+NKm7Lue8O7z+L8w/He8ALuE2EAH4fX4M4GF4310MWRt5vll9 6PCki79N2L6WZinJVW7ItKgBAoP0B7DtjKw== X-Received: by 2002:a05:6512:2346:b0:4f6:1307:80b0 with SMTP id p6-20020a056512234600b004f6130780b0mr6488975lfu.12.1687249776570; Tue, 20 Jun 2023 01:29:36 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7L/s/vGCRbb/ufC47Bq18TT0GV1xgdD/50kR2a15RKZWZJDNi0+bXyCYp0Dr8UCoZIEWBTcol2d6b8K5rlYBM= X-Received: by 2002:a05:6512:2346:b0:4f6:1307:80b0 with SMTP id p6-20020a056512234600b004f6130780b0mr6488971lfu.12.1687249776246; Tue, 20 Jun 2023 01:29:36 -0700 (PDT) MIME-Version: 1.0 References: <20230609120917.294304-1-jwakely@redhat.com> In-Reply-To: From: Jonathan Wakely Date: Tue, 20 Jun 2023 09:29:24 +0100 Message-ID: Subject: Re: [committed] libstdc++: Optimize std::to_array for trivial types [PR110167] To: Patrick Palka Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="0000000000008f973205fe8b73e8" X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE,URI_HEX autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --0000000000008f973205fe8b73e8 Content-Type: text/plain; charset="UTF-8" On Tue, 20 Jun 2023 at 01:54, Patrick Palka wrote: > On Fri, 9 Jun 2023, Jonathan Wakely via Libstdc++ wrote: > > > Tested powerpc64le-linux. Pushed to trunk. > > > > This makes sense to backport after some soak time on trunk. > > > > -- >8 -- > > > > As reported in PR libstdc++/110167, std::to_array compiles extremely > > slowly for very large arrays. It needs to instantiate a very large > > specialization of std::index_sequence and then create a very large > > aggregate initializer from the pack expansion. For trivial types we can > > simply default-initialize the std::array and then use memcpy to copy the > > values. For non-trivial types we need to use the existing > > implementation, despite the compilation cost. > > > > As also noted in the PR, using a generic lambda instead of the > > __to_array helper compiles faster since gcc-13. It also produces > > slightly smaller code at -O1, due to additional inlining. The code at > > -Os, -O2 and -O3 seems to be the same. This new implementation requires > > __cpp_generic_lambdas >= 201707L (i.e. P0428R2) but that is supported > > since Clang 10 and since Intel icc 2021.5.0 (and since GCC 10.1). > > > > libstdc++-v3/ChangeLog: > > > > PR libstdc++/110167 > > * include/std/array (to_array): Initialize arrays of trivial > > types using memcpy. For non-trivial types, use lambda > > expressions instead of a separate helper function. > > (__to_array): Remove. > > * testsuite/23_containers/array/creation/110167.cc: New test. > > --- > > libstdc++-v3/include/std/array | 53 +++++++++++++------ > > .../23_containers/array/creation/110167.cc | 14 +++++ > > 2 files changed, 51 insertions(+), 16 deletions(-) > > create mode 100644 > libstdc++-v3/testsuite/23_containers/array/creation/110167.cc > > > > diff --git a/libstdc++-v3/include/std/array > b/libstdc++-v3/include/std/array > > index 70280c1beeb..b791d86ddb2 100644 > > --- a/libstdc++-v3/include/std/array > > +++ b/libstdc++-v3/include/std/array > > @@ -414,19 +414,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > return std::move(std::get<_Int>(__arr)); > > } > > > > -#if __cplusplus > 201703L > > +#if __cplusplus >= 202002L && __cpp_generic_lambdas >= 201707L > > #define __cpp_lib_to_array 201907L > > - > > - template > > - constexpr array, sizeof...(_Idx)> > > - __to_array(_Tp (&__a)[sizeof...(_Idx)], index_sequence<_Idx...>) > > - { > > - if constexpr (_Move) > > - return {{std::move(__a[_Idx])...}}; > > - else > > - return {{__a[_Idx]...}}; > > - } > > - > > template > > [[nodiscard]] > > constexpr array, _Nm> > > @@ -436,8 +425,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > static_assert(!is_array_v<_Tp>); > > static_assert(is_constructible_v<_Tp, _Tp&>); > > if constexpr (is_constructible_v<_Tp, _Tp&>) > > - return __to_array(__a, make_index_sequence<_Nm>{}); > > - __builtin_unreachable(); // FIXME: see PR c++/91388 > > + { > > + if constexpr (is_trivial_v<_Tp> && _Nm != 0) > > redundant _Nm != 0 test? > Ah yes, I added it below to ensure we don't use memcpy with a null __arr.data() and forgot to remove it here. > > > + { > > + array, _Nm> __arr; > > + if (!__is_constant_evaluated() && _Nm != 0) > > + __builtin_memcpy(__arr.data(), __a, sizeof(__a)); > > + else > > + for (size_t __i = 0; __i < _Nm; ++__i) > > + __arr._M_elems[__i] = __a[__i]; > > + return __arr; > > + } > > + else > > + return [&__a](index_sequence<_Idx...>) { > > + return array, _Nm>{{ __a[_Idx]... }}; > > + }(make_index_sequence<_Nm>{}); > > + } > > + else > > + __builtin_unreachable(); // FIXME: see PR c++/91388 > > } > > > > template > > @@ -449,8 +454,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > static_assert(!is_array_v<_Tp>); > > static_assert(is_move_constructible_v<_Tp>); > > if constexpr (is_move_constructible_v<_Tp>) > > - return __to_array<1>(__a, make_index_sequence<_Nm>{}); > > - __builtin_unreachable(); // FIXME: see PR c++/91388 > > + { > > + if constexpr (is_trivial_v<_Tp>) > > + { > > + array, _Nm> __arr; > > + if (!__is_constant_evaluated() && _Nm != 0) > > + __builtin_memcpy(__arr.data(), __a, sizeof(__a)); > > + else > > + for (size_t __i = 0; __i < _Nm; ++__i) > > + __arr._M_elems[__i] = std::move(__a[__i]); > > IIUC this std::move is unnecessary for trivial arrays? > Good point, thanks. That makes the lvalue and rvalue overloads identical for trivial types. It seems a shame to duplicate the code, so the rvalue one could do: if constexpr (is_trivial_v<_Tp>) return std::to_array<_Tp, _Num>(__a); else But that would imply an extra function call at -O0, and repeating overload resolution. Since the duplicated code is just a single function call to memcpy, this probably isn't an improvement. I'll test and push this: libstdc++: Remove redundant code in std::to_array libstdc++-v3/ChangeLog: * include/std/array (to_array(T(&)[N])): Remove redundant condition. (to_array(T(&&)[N])): Remove redundant std::move. diff --git a/libstdc++-v3/include/std/array b/libstdc++-v3/include/std/array index b791d86ddb2..ad36cdad6d2 100644 --- a/libstdc++-v3/include/std/array +++ b/libstdc++-v3/include/std/array @@ -426,7 +426,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_constructible_v<_Tp, _Tp&>); if constexpr (is_constructible_v<_Tp, _Tp&>) { - if constexpr (is_trivial_v<_Tp> && _Nm != 0) + if constexpr (is_trivial_v<_Tp>) { array, _Nm> __arr; if (!__is_constant_evaluated() && _Nm != 0) @@ -462,7 +462,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __builtin_memcpy(__arr.data(), __a, sizeof(__a)); else for (size_t __i = 0; __i < _Nm; ++__i) - __arr._M_elems[__i] = std::move(__a[__i]); + __arr._M_elems[__i] = __a[__i]; return __arr; } else --0000000000008f973205fe8b73e8--