Message ID | oree1h91sd.fsf@lxoliva.fsfla.org |
---|---|
State | Superseded |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DB7B53857C49 for <patchwork@sourceware.org>; Thu, 28 Apr 2022 06:10:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DB7B53857C49 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1651126232; bh=/XMzSqGIPweiBQN6KKGqSkax3AvxotHKx7FYcOHo+PU=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=menKpkegO4/itzyixlVg91bTmKTWOPKF6tTceFngB6GdEWUpyCR2Dr92dcOTn04HR fjvXI7GjA/W3R4h1fxQczzpJ//XehyOClalTBaAOMmqFTFPyC8Z18DlWv4m2YvBY06 Mni1GRtoMbQQb20JGtfFrDxZY+pBoQHD9/4GPoxk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTPS id 7102E3858D1E; Thu, 28 Apr 2022 06:10:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7102E3858D1E Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4222211622E; Thu, 28 Apr 2022 02:10:03 -0400 (EDT) X-Virus-Scanned: Debian amavisd-new at gnat.com X-Amavis-Alert: BAD HEADER SECTION, Duplicate header field: "Cc" Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id UJjyrFZLYgah; Thu, 28 Apr 2022 02:10:03 -0400 (EDT) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id CC1E211621C; Thu, 28 Apr 2022 02:10:02 -0400 (EDT) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 23S69shv2550574 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Apr 2022 03:09:54 -0300 To: gcc-patches@gcc.gnu.org Subject: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics Organization: Free thinker, does not speak for AdaCore Date: Thu, 28 Apr 2022 03:09:54 -0300 Message-ID: <oree1h91sd.fsf@lxoliva.fsfla.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Alexandre Oliva via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Alexandre Oliva <oliva@adacore.com> Cc: libstdc++@gcc.gnu.org, Segher Boessenkool <segher@kernel.crashing.org>, David Edelsohn <dje.gcc@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
libstdc++: ppc: conditionalize vsx-only simd intrinsics
|
|
Commit Message
Alexandre Oliva
April 28, 2022, 6:09 a.m. UTC
libstdc++'s bits/simd.h section for PPC (Altivec) defines various intrinsic vector types that are only available along with VSX: 64-bit long double, double, (un)signed long long, and 64-bit (un)signed long. experimental/simd/standard_abi_usable{,_2}.cc tests error out reporting the unmet requirements when the target cpu doesn't enable VSX. Make the reported instrinsic types conditional on VSX so that <experimental/simd> can be used on ppc variants that do not have VSX support. Regstrapped on powerpc64el-linux-gnu. Ok to install? This is also relevant for gcc-11. Tested with x86_64-linux-gnu-x-ppc64-vx7r2. Ok for gcc-11? for libstdc++-v3/ChangeLog * include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX for double, long long, and 64-bit long and 64-bit long double intrinsic types. --- libstdc++-v3/include/experimental/bits/simd.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Comments
Hi! On Thu, Apr 28, 2022 at 03:09:54AM -0300, Alexandre Oliva wrote: > libstdc++'s bits/simd.h section for PPC (Altivec) defines various > intrinsic vector types that are only available along with VSX: 64-bit > long double, double, (un)signed long long, and 64-bit (un)signed long. > > experimental/simd/standard_abi_usable{,_2}.cc tests error out reporting > the unmet requirements when the target cpu doesn't enable VSX. Make the > reported instrinsic types conditional on VSX so that <experimental/simd> > can be used on ppc variants that do not have VSX support. > * include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX > for double, long long, and 64-bit long and 64-bit long double > intrinsic types. > --- a/libstdc++-v3/include/experimental/bits/simd.h > +++ b/libstdc++-v3/include/experimental/bits/simd.h > @@ -2430,17 +2430,23 @@ template <typename _Tp> > template <> \ > struct __intrinsic_type_impl<_Tp> { using type = __vector _Tp; } > _GLIBCXX_SIMD_PPC_INTRIN(float); > +# ifdef __VSX__ No space after # (here and everywhere else). > #ifndef __VSX__ > - static_assert(!is_same_v<_Tp, double>, > - "no __intrinsic_type support for double on PPC w/o VSX"); > + static_assert(!(is_same_v<_Tp, double> > + || (_S_is_ldouble && sizeof(long double) == sizeof(double))), > + "no __intrinsic_type support for [long] double on PPC w/o VSX"); > #endif This change isn't in the changelog. The message should not say "long double" without qualifying it as "64-bit long double". It is probably best to just keep the message as-is, the other possibilities for long double aren't supported either, not even with VSX, and all this becomes much too complicated to put in a simple error message. It confuses the poor user, as well. Alternatively you can make it two separate error messages, one for double, one for 64-bit long double. Okay for trunk without this part of the patch, and the spaces after the hash signs deleted. Or send a new patch with the "[long] double" thing fixed? Thanks! Segher
On Thursday, 28 April 2022 08:09:54 CEST Alexandre Oliva via Gcc-patches wrote: > libstdc++'s bits/simd.h section for PPC (Altivec) defines various > intrinsic vector types that are only available along with VSX: 64-bit > long double, double, (un)signed long long, and 64-bit (un)signed long. Oh, so uttering `__vector double` is ill-formed (now) without VSX? I'm fairly certain I tested without VSX and the __intrinsic_type_impl definitions were fine. > experimental/simd/standard_abi_usable{,_2}.cc tests error out reporting > the unmet requirements when the target cpu doesn't enable VSX. Make the > reported instrinsic types conditional on VSX so that <experimental/simd> > can be used on ppc variants that do not have VSX support. IIRC this will break other valid uses. You'd have to run `make check-simd` (see libstdc++-v3/testsuite/experimental/simd/README.md) to be certain nothing breaks. I will also take a look. > Regstrapped on powerpc64el-linux-gnu. Ok to install? > > This is also relevant for gcc-11. Tested with > x86_64-linux-gnu-x-ppc64-vx7r2. Ok for gcc-11? > > > for libstdc++-v3/ChangeLog > > * include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX > for double, long long, and 64-bit long and 64-bit long double > intrinsic types. > --- > libstdc++-v3/include/experimental/bits/simd.h | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/libstdc++-v3/include/experimental/bits/simd.h > b/libstdc++-v3/include/experimental/bits/simd.h index > 82e9841195e1d..66c07127ec435 100644 > --- a/libstdc++-v3/include/experimental/bits/simd.h > +++ b/libstdc++-v3/include/experimental/bits/simd.h > @@ -2430,17 +2430,23 @@ template <typename _Tp> > template <> > \ struct __intrinsic_type_impl<_Tp> { using type = __vector _Tp; } > _GLIBCXX_SIMD_PPC_INTRIN(float); > +# ifdef __VSX__ > _GLIBCXX_SIMD_PPC_INTRIN(double); > +# endif > _GLIBCXX_SIMD_PPC_INTRIN(signed char); > _GLIBCXX_SIMD_PPC_INTRIN(unsigned char); > _GLIBCXX_SIMD_PPC_INTRIN(signed short); > _GLIBCXX_SIMD_PPC_INTRIN(unsigned short); > _GLIBCXX_SIMD_PPC_INTRIN(signed int); > _GLIBCXX_SIMD_PPC_INTRIN(unsigned int); > +# if defined __VSX__ || __LONG_WIDTH__ == 32 > _GLIBCXX_SIMD_PPC_INTRIN(signed long); > _GLIBCXX_SIMD_PPC_INTRIN(unsigned long); > +# endif > +# ifdef __VSX__ > _GLIBCXX_SIMD_PPC_INTRIN(signed long long); > _GLIBCXX_SIMD_PPC_INTRIN(unsigned long long); > +# endif > #undef _GLIBCXX_SIMD_PPC_INTRIN > > template <typename _Tp, size_t _Bytes> > @@ -2452,8 +2458,9 @@ template <typename _Tp, size_t _Bytes> > static_assert(!(_S_is_ldouble && sizeof(long double) > sizeof(double)), > "no __intrinsic_type support for long double on PPC"); > #ifndef __VSX__ > - static_assert(!is_same_v<_Tp, double>, > - "no __intrinsic_type support for double on PPC w/o VSX"); > + static_assert(!(is_same_v<_Tp, double> > + || (_S_is_ldouble && sizeof(long double) == sizeof(double))), > + "no __intrinsic_type support for [long] double on PPC w/o VSX"); The missing condition here was an incorrect omission. With -mlong-double-64 and without VSX no assertion caught the issue. IIRC, a user won't get to see this error message unless there's a bug in the simd library implementation, so the error message is good enough for me. (It's talking about __intrinsic_type, the user would be lost in any case.) > #endif > using type = > typename __intrinsic_type_impl<
On Thu, Apr 28, 2022 at 03:56:18PM +0200, Matthias Kretz wrote: > On Thursday, 28 April 2022 08:09:54 CEST Alexandre Oliva via Gcc-patches > wrote: > > libstdc++'s bits/simd.h section for PPC (Altivec) defines various > > intrinsic vector types that are only available along with VSX: 64-bit > > long double, double, (un)signed long long, and 64-bit (un)signed long. > > Oh, so uttering `__vector double` is ill-formed (now) without VSX? I'm fairly > certain I tested without VSX and the __intrinsic_type_impl definitions were > fine. It always was undefined (and did not work): error: use of 'double' in AltiVec types is invalid without '-mvsx' Segher
On Apr 28, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Thu, Apr 28, 2022 at 03:09:54AM -0300, Alexandre Oliva wrote: >> +# ifdef __VSX__ > No space after # (here and everywhere else). 'k, thanks >> + "no __intrinsic_type support for [long] double on PPC w/o VSX"); > This change isn't in the changelog. It is, it's just that long double is dealt with differently from the other types, so requiring VSX for it takes a different form: > * include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX > for double, long long, and 64-bit long and 64-bit long double > intrinsic types. ^^^^^^^^^^^^^^^^^^ > The message should not say "long > double" without qualifying it as "64-bit long double". How about qualifying both such messages as in this incremental patchlet: @@ -2456,11 +2456,11 @@ template <typename _Tp, size_t _Bytes> static constexpr bool _S_is_ldouble = is_same_v<_Tp, long double>; // allow _Tp == long double with -mlong-double-64 static_assert(!(_S_is_ldouble && sizeof(long double) > sizeof(double)), - "no __intrinsic_type support for long double on PPC"); + "no __intrinsic_type support for 128-bit floating point on PPC"); #ifndef __VSX__ static_assert(!(is_same_v<_Tp, double> || (_S_is_ldouble && sizeof(long double) == sizeof(double))), - "no __intrinsic_type support for [long] double on PPC w/o VSX"); + "no __intrinsic_type support for 64-bit floating point on PPC w/o VSX"); #endif using type = typename __intrinsic_type_impl< > Okay for trunk without this part of the patch, and the spaces after the > hash signs deleted. Or send a new patch with the "[long] double" thing > fixed? Thanks, awaiting feedback on the suggestion above to post the consolidated patch.
On Friday, 29 April 2022 03:53:40 CEST Alexandre Oliva via Gcc-patches wrote: > Thanks, awaiting feedback on the suggestion above to post the consolidated > patch. LGTM. I think this improves clarity for the __intrisic_type static assertions significantly. And don't bother with my other mail. If `__vector double` without VSX was always ill-formed then I must be misremembering something. Cheers, Matthias
Hi! On Thu, Apr 28, 2022 at 10:53:40PM -0300, Alexandre Oliva wrote: > On Apr 28, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Apr 28, 2022 at 03:09:54AM -0300, Alexandre Oliva wrote: > >> + "no __intrinsic_type support for [long] double on PPC w/o VSX"); > > This change isn't in the changelog. > > It is, it's just that long double is dealt with differently from the > other types, so requiring VSX for it takes a different form: It changes a diagnostic message. Incorrectly, even. The changelog does not talk about this. The only thing I use changelogs for nowadays is to help reviewing code. So when things are missing it stands out like a sore thumb. > > * include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX > > for double, long long, and 64-bit long and 64-bit long double > > intrinsic types. ^^^^^^^^^^^^^^^^^^ Yes, that does not talk about changing a diagnostic message. > > The message should not say "long > > double" without qualifying it as "64-bit long double". > > How about qualifying both such messages as in this incremental patchlet: Send full patches always please. Not patches relative to some non-existing state :-) Thanks, Segher
diff --git a/libstdc++-v3/include/experimental/bits/simd.h b/libstdc++-v3/include/experimental/bits/simd.h index 82e9841195e1d..66c07127ec435 100644 --- a/libstdc++-v3/include/experimental/bits/simd.h +++ b/libstdc++-v3/include/experimental/bits/simd.h @@ -2430,17 +2430,23 @@ template <typename _Tp> template <> \ struct __intrinsic_type_impl<_Tp> { using type = __vector _Tp; } _GLIBCXX_SIMD_PPC_INTRIN(float); +# ifdef __VSX__ _GLIBCXX_SIMD_PPC_INTRIN(double); +# endif _GLIBCXX_SIMD_PPC_INTRIN(signed char); _GLIBCXX_SIMD_PPC_INTRIN(unsigned char); _GLIBCXX_SIMD_PPC_INTRIN(signed short); _GLIBCXX_SIMD_PPC_INTRIN(unsigned short); _GLIBCXX_SIMD_PPC_INTRIN(signed int); _GLIBCXX_SIMD_PPC_INTRIN(unsigned int); +# if defined __VSX__ || __LONG_WIDTH__ == 32 _GLIBCXX_SIMD_PPC_INTRIN(signed long); _GLIBCXX_SIMD_PPC_INTRIN(unsigned long); +# endif +# ifdef __VSX__ _GLIBCXX_SIMD_PPC_INTRIN(signed long long); _GLIBCXX_SIMD_PPC_INTRIN(unsigned long long); +# endif #undef _GLIBCXX_SIMD_PPC_INTRIN template <typename _Tp, size_t _Bytes> @@ -2452,8 +2458,9 @@ template <typename _Tp, size_t _Bytes> static_assert(!(_S_is_ldouble && sizeof(long double) > sizeof(double)), "no __intrinsic_type support for long double on PPC"); #ifndef __VSX__ - static_assert(!is_same_v<_Tp, double>, - "no __intrinsic_type support for double on PPC w/o VSX"); + static_assert(!(is_same_v<_Tp, double> + || (_S_is_ldouble && sizeof(long double) == sizeof(double))), + "no __intrinsic_type support for [long] double on PPC w/o VSX"); #endif using type = typename __intrinsic_type_impl<