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.133.124]) by sourceware.org (Postfix) with ESMTPS id DBF963858D32 for ; Tue, 20 Jun 2023 14:37:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DBF963858D32 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=1687271853; 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=FTl+0x7NNNYOd2gLAb2eS6Ey1a+7zwj9QL+DN1zB+YQ=; b=L+hHjUg6H+FGlybaRfajhutQ2thB/qXG6F0vTiAmskbp8qpwomflCbRlZGsaI6/sQZ7cqP UmGwfTZRIUilLdJNvGJa4pFPd1LBJ4FtEDFw83IcSylZvUJFeUwwiSl4OWmDWuDi5DZixu +0lUzUUpui2U2ucNRtEczOWpIxkNZ40= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-136-raEMyRvtOpaqMt1Piyg4rQ-1; Tue, 20 Jun 2023 10:37:30 -0400 X-MC-Unique: raEMyRvtOpaqMt1Piyg4rQ-1 Received: by mail-lj1-f197.google.com with SMTP id 38308e7fff4ca-2b1d8fa4629so37230591fa.0 for ; Tue, 20 Jun 2023 07:37:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687271846; x=1689863846; 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=+QfPEkvb16zzS3avhLFUovchb2pntMN5GljOvYv9OTQ=; b=GuAeVBMqalbu+XKtU4lsvSeEH55ejKCRbZ/KYYcoTcL7aivDIYNbnajZ8bFk4pzC+l VrinXKA/4r+/WULBzYkaj8f4y3sPcpytSvpgbeOB4lx/So08pF7lFw3OUsYwQ9Iv0pcJ uOW2IcEgMhHceUgHDV38OXju/jO6OaWkqNgxaXs8fPjbX5hCNLqh286H9AqenK1DEm6R MywhRgicUtljlhQioddF57GtfDA7fbbbfU5oJTbYYWYfQlohbu4RSDFQH8yR8gcH8mbM ++9WjVu24Gz0FYqupZLcGmsPVwBjlhnmXC2UhmJuDtCfaVQ8FiKXx+mFhNnMn5DUEHtf yuNQ== X-Gm-Message-State: AC+VfDzEnAuo80dPxAduvwFNYuxYSMJy0b5pSZcaDXXHZq0qiNyGio0h tazFUpMP17dzYrfsXcRvd7pY97Wf0vfemhR+w/gvrcw7ZqHybsBFs85xtPe5adodam+l5DlU/xz RNd/5NqNmC9RL1NzbnB5qWeeAMxEUTvk1Gg== X-Received: by 2002:a05:651c:1036:b0:2b4:49e7:d43e with SMTP id w22-20020a05651c103600b002b449e7d43emr8275740ljm.1.1687271846680; Tue, 20 Jun 2023 07:37:26 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4dELV7OnHR97CMcV/l0WhhZVklr66/4hFbvRAQ07n3tPVOpDqQrdQiB1y/XRx9FQHoJHdzz5OKrdItAWnaIUg= X-Received: by 2002:a05:651c:1036:b0:2b4:49e7:d43e with SMTP id w22-20020a05651c103600b002b449e7d43emr8275729ljm.1.1687271846333; Tue, 20 Jun 2023 07:37:26 -0700 (PDT) MIME-Version: 1.0 References: <20230609120917.294304-1-jwakely@redhat.com> In-Reply-To: From: Jonathan Wakely Date: Tue, 20 Jun 2023 15:37:15 +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="0000000000000a657a05fe909782" 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,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,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: --0000000000000a657a05fe909782 Content-Type: text/plain; charset="UTF-8" On Tue, 20 Jun 2023 at 09:29, Jonathan Wakely wrote: > > > 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: > Pushed as r14-1995-gb4f1e4a6443802, thanks for the review. > 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 > > --0000000000000a657a05fe909782--