| Message ID | oree1h91sd.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: ppc: conditionalize vsx-only simd intrinsics | expand |
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<
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(-)