public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
@ 2022-04-28  6:09 Alexandre Oliva
  2022-04-28 13:03 ` Segher Boessenkool
  2022-04-28 13:56 ` [PATCH] " Matthias Kretz
  0 siblings, 2 replies; 11+ messages in thread
From: Alexandre Oliva @ 2022-04-28  6:09 UTC (permalink / raw)
  To: gcc-patches; +Cc: libstdc++, David Edelsohn, Segher Boessenkool


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(-)

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<


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-04-28  6:09 [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics Alexandre Oliva
@ 2022-04-28 13:03 ` Segher Boessenkool
  2022-04-29  1:53   ` Alexandre Oliva
  2022-04-28 13:56 ` [PATCH] " Matthias Kretz
  1 sibling, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2022-04-28 13:03 UTC (permalink / raw)
  To: Alexandre Oliva; +Cc: gcc-patches, libstdc++, David Edelsohn

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-04-28  6:09 [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics Alexandre Oliva
  2022-04-28 13:03 ` Segher Boessenkool
@ 2022-04-28 13:56 ` Matthias Kretz
  2022-04-28 16:06   ` Segher Boessenkool
  1 sibling, 1 reply; 11+ messages in thread
From: Matthias Kretz @ 2022-04-28 13:56 UTC (permalink / raw)
  To: gcc-patches
  Cc: libstdc++, Segher Boessenkool, David Edelsohn, Alexandre Oliva

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<


-- 
──────────────────────────────────────────────────────────────────────────
 Dr. Matthias Kretz                           https://mattkretz.github.io
 GSI Helmholtz Centre for Heavy Ion Research               https://gsi.de
 stdₓ::simd
──────────────────────────────────────────────────────────────────────────




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-04-28 13:56 ` [PATCH] " Matthias Kretz
@ 2022-04-28 16:06   ` Segher Boessenkool
  0 siblings, 0 replies; 11+ messages in thread
From: Segher Boessenkool @ 2022-04-28 16:06 UTC (permalink / raw)
  To: Matthias Kretz; +Cc: gcc-patches, libstdc++, David Edelsohn, Alexandre Oliva

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-04-28 13:03 ` Segher Boessenkool
@ 2022-04-29  1:53   ` Alexandre Oliva
  2022-04-29 18:44     ` Matthias Kretz
  2022-05-02 21:36     ` Segher Boessenkool
  0 siblings, 2 replies; 11+ messages in thread
From: Alexandre Oliva @ 2022-04-29  1:53 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: gcc-patches, libstdc++, David Edelsohn

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.

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-04-29  1:53   ` Alexandre Oliva
@ 2022-04-29 18:44     ` Matthias Kretz
  2022-05-02 21:36     ` Segher Boessenkool
  1 sibling, 0 replies; 11+ messages in thread
From: Matthias Kretz @ 2022-04-29 18:44 UTC (permalink / raw)
  To: Segher Boessenkool, gcc-patches
  Cc: libstdc++, gcc-patches, David Edelsohn, Alexandre Oliva

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

-- 
──────────────────────────────────────────────────────────────────────────
 Dr. Matthias Kretz                           https://mattkretz.github.io
 GSI Helmholtz Centre for Heavy Ion Research               https://gsi.de
 stdₓ::simd
──────────────────────────────────────────────────────────────────────────

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-04-29  1:53   ` Alexandre Oliva
  2022-04-29 18:44     ` Matthias Kretz
@ 2022-05-02 21:36     ` Segher Boessenkool
  2022-05-03  2:00       ` [PATCH v2] " Alexandre Oliva
  1 sibling, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2022-05-02 21:36 UTC (permalink / raw)
  To: Alexandre Oliva; +Cc: gcc-patches, libstdc++, David Edelsohn

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-05-02 21:36     ` Segher Boessenkool
@ 2022-05-03  2:00       ` Alexandre Oliva
  2022-05-06 16:14         ` Segher Boessenkool
  0 siblings, 1 reply; 11+ messages in thread
From: Alexandre Oliva @ 2022-05-03  2:00 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: libstdc++, gcc-patches, David Edelsohn

On May  2, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Send full patches always please.

I'll try to remember that.  In case I fail, I hope you won't mind too
much reminding me.

(You'd also asked me not to send patches as followups, but...  revised
versions of a patch still belong in the same thread, right?)

Here's the updated full patch.  I tried to find earlier references to
this partial specialization of template struct __intrinsic_type in
ChangeLogs, to no avail.  (That was why I went for the more abstract
ChangeLog entry before.)


libstdc++: ppc: conditionalize vsx-only simd intrinsics

From: Alexandre Oliva <oliva@adacore.com>

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.

Regstrapping on powerpc64le-linux-gnu.  Ok for trunk?  gcc-12?  gcc-11?


for  libstdc++-v3/ChangeLog

	* include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX
	for double, long long, and 64-bit long intrinsic types.
	[__ALTIVEC__] (__intrinsic_type): Mention 128-bit in
	preexisting long double diagnostic, adjust no-VSX double
	diagnostic to cover 64-bit long double as well.
---
 libstdc++-v3/include/experimental/bits/simd.h |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/libstdc++-v3/include/experimental/bits/simd.h b/libstdc++-v3/include/experimental/bits/simd.h
index 82e9841195e1d..349726a16d71c 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>
@@ -2450,10 +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>,
-		  "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 64-bit floating point on PPC w/o VSX");
 #endif
     using type =
       typename __intrinsic_type_impl<


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-05-03  2:00       ` [PATCH v2] " Alexandre Oliva
@ 2022-05-06 16:14         ` Segher Boessenkool
  2022-05-06 17:05           ` Jonathan Wakely
  0 siblings, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2022-05-06 16:14 UTC (permalink / raw)
  To: Alexandre Oliva; +Cc: libstdc++, gcc-patches, David Edelsohn

On Mon, May 02, 2022 at 11:00:02PM -0300, Alexandre Oliva wrote:
> On May  2, 2022, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > Send full patches always please.
> 
> I'll try to remember that.  In case I fail, I hope you won't mind too
> much reminding me.
> 
> (You'd also asked me not to send patches as followups, but...  revised
> versions of a patch still belong in the same thread, right?)

No.  This makes it much harder to keep versions apart, even worse if you
use patchwork or similar.

The mail with the patch should be the head of a mail thread (or just
below the head if that is a cover mail), and everything below that is
discussion related to that patch.  The mail with the patch should be
ready to be committed as-is, so it should not have mangled whitespace
or encoding, should have proper commit message, etc.  This a) makes it
possible to review what will actualle be committed, and b) it makes it
possible for someone else to commit it (and that includes patch test
systems, often mistakenly called CI or CD, which are completely
different things).

> libstdc++'s bits/simd.h section for PPC (Altivec) defines various

(Spelling: PowerPC, AltiVec.  But this isn't about AltiVec really
anyway?)

> 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.

> +#if defined __VSX__ || __LONG_WIDTH__ == 32
>  _GLIBCXX_SIMD_PPC_INTRIN(signed long);
>  _GLIBCXX_SIMD_PPC_INTRIN(unsigned long);
> +#endif

Is __LONG_WIDTH__ the right macro to use here?  Nothing else in
libstdc++v3 uses it.  "__CHAR_BIT__ * __SIZEOF_LONG__" is the usual
thing to do.  Is __LONG_WIDTH__ always defined anyway?

> @@ -2450,10 +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");

You might do s/PPC/PowerPC/ at the same time.

Okay for trunk with __LONG_WIDTH__ dealt with.  Okay for the branches
a week or so after that.  Thanks!


Segher

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-05-06 16:14         ` Segher Boessenkool
@ 2022-05-06 17:05           ` Jonathan Wakely
  2022-05-06 18:24             ` [PATCH v3] " Alexandre Oliva
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2022-05-06 17:05 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Alexandre Oliva, libstdc++, gcc-patches, David Edelsohn

On Fri, 6 May 2022 at 17:17, Segher Boessenkool wrote:
> > +#if defined __VSX__ || __LONG_WIDTH__ == 32
> >  _GLIBCXX_SIMD_PPC_INTRIN(signed long);
> >  _GLIBCXX_SIMD_PPC_INTRIN(unsigned long);
> > +#endif
>
> Is __LONG_WIDTH__ the right macro to use here?  Nothing else in
> libstdc++v3 uses it.  "__CHAR_BIT__ * __SIZEOF_LONG__" is the usual
> thing to do.  Is __LONG_WIDTH__ always defined anyway?

Presumably it could be simply __SIZEOF_LONG__ == 4 if this is
PowerPC-specific code where CHAR_BIT==8 is always true?

We don't need to consider hypothetical targets where CHAR_BIT!=8 if we
already know the target is some version of PowerPC.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] libstdc++: ppc: conditionalize vsx-only simd intrinsics
  2022-05-06 17:05           ` Jonathan Wakely
@ 2022-05-06 18:24             ` Alexandre Oliva
  0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Oliva @ 2022-05-06 18:24 UTC (permalink / raw)
  To: Jonathan Wakely
  Cc: Segher Boessenkool, libstdc++, gcc-patches, David Edelsohn

On May  6, 2022, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:

> On Fri, 6 May 2022 at 17:17, Segher Boessenkool wrote:
>> > +#if defined __VSX__ || __LONG_WIDTH__ == 32
>> >  _GLIBCXX_SIMD_PPC_INTRIN(signed long);
>> >  _GLIBCXX_SIMD_PPC_INTRIN(unsigned long);
>> > +#endif
>> 
>> Is __LONG_WIDTH__ the right macro to use here?  Nothing else in
>> libstdc++v3 uses it.  "__CHAR_BIT__ * __SIZEOF_LONG__" is the usual
>> thing to do.  Is __LONG_WIDTH__ always defined anyway?

> Presumably it could be simply __SIZEOF_LONG__ == 4 if this is
> PowerPC-specific code where CHAR_BIT==8 is always true?

SGTM.  Here's the adjusted patch I'm checking in momentarily, trunk
first, then gcc-12 and gcc-11 after a week or so.  Thanks,


libstdc++: ppc: conditionalize vsx-only simd intrinsics

libstdc++'s bits/simd.h section for PowerPC, guarded by __ALTIVEC__,
defines various intrinsic vector types that are only available 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 PowerPC variants that do not
support VSX.


for  libstdc++-v3/ChangeLog

	* include/experimental/bits/simd.h [__ALTIVEC__]: Require VSX
	for double, long long, and 64-bit long intrinsic types.
	[__ALTIVEC__] (__intrinsic_type): Mention 128-bit in
	preexisting long double diagnostic, adjust no-VSX double
	diagnostic to cover 64-bit long double as well.
---
 libstdc++-v3/include/experimental/bits/simd.h |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/libstdc++-v3/include/experimental/bits/simd.h b/libstdc++-v3/include/experimental/bits/simd.h
index 82e9841195e1d..b0226fa4c5304 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__ || __SIZEOF_LONG__ == 4
 _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>
@@ -2450,10 +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 PowerPC");
 #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 64-bit floating point on PowerPC w/o VSX");
 #endif
     using type =
       typename __intrinsic_type_impl<


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-05-06 18:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-28  6:09 [PATCH] libstdc++: ppc: conditionalize vsx-only simd intrinsics Alexandre Oliva
2022-04-28 13:03 ` Segher Boessenkool
2022-04-29  1:53   ` Alexandre Oliva
2022-04-29 18:44     ` Matthias Kretz
2022-05-02 21:36     ` Segher Boessenkool
2022-05-03  2:00       ` [PATCH v2] " Alexandre Oliva
2022-05-06 16:14         ` Segher Boessenkool
2022-05-06 17:05           ` Jonathan Wakely
2022-05-06 18:24             ` [PATCH v3] " Alexandre Oliva
2022-04-28 13:56 ` [PATCH] " Matthias Kretz
2022-04-28 16:06   ` Segher Boessenkool

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).