[committed] Re: [PATCH] libstdc++: Add missing constexpr to simd
I pushed the attached patch.
I kept the operator names... too late, there were already operator names in
the stdx::simd implemenation anyway. ;)
- Matthias
On Monday, 22 May 2023 22:51:49 CEST Jonathan Wakely wrote:
> On Mon, 22 May 2023 at 21:27, Matthias Kretz wrote:
> > On Monday, 22 May 2023 18:25:15 CEST Jonathan Wakely wrote:
> > > I note that using if (not __builtin_constant_evaluated()) will fail if
> > > compiled with -fno-operator-names, which is why we don't use 'not',
> >
> > 'and',
> >
> > > etc. elsewhere in libstdc++. I don't know if (or why) anybody uses that
> > > option though, so I don't think you need to hange anything in
> > > stdx::simd.
> >
> > Ah, I just recently convinced myself that "operator-names" are more
> > readable
> > (=> easier to maintain).
>
> I tend to agree, but every time I decide to start using them some testcases
> start to fail and I remember why we don't use them :-(
>
> > But OTOH a mix isn't necessarily better. I'm fine
> > with keeping it consistent.
> >
> > > > * subscripting vector builtins is not allowed in constant expressions
> > >
> > > Is that just because nobody made it work (yet)?
> >
> > That is a good question. I guess I should open a PR.
> >
> > > * if the implementation needs/uses memcpy
> > >
> > > > * if the implementation would otherwise call SIMD intrinsics/builtins
> > >
> > > The indentation looks off here and in the _M_set member function
> >
> > following
> >
> > > it:
> > Yes. I had to put an #if between an else and an if. Looks like this:
> > else
> >
> > #ifdef _GLIBCXX_SIMD_USE_ALIASING_LOADS
> >
> > if (not __builtin_is_constant_evaluated())
> > return reinterpret_cast*>(this)[__i];
> >
> > else
> >
> > #endif
> >
> > if constexpr (__is_scalar_abi<_Abi0>())
>
> Ah yes, so the if is indented two spaces from the else above it.
> What looks wrong to me is that the return is the at the same indentation as
> the if controlling it.
>
> > Should the `if` be aligned to the `else` instead?
>
> How about moving the two else tokens?
>
> #ifdef _GLIBCXX_SIMD_USE_ALIASING_LOADS
>else if (not __builtin_is_constant_evaluated())
> return reinterpret_cast*>(this)[__i];
> #endif
>else if constexpr (__is_scalar_abi<_Abi0>())
>
> I think that avoids the issue.
>
> > > Are the copyright years on
> > > testsuite/experimental/simd/pr109261_constexpr_simd.cc correct, or just
> > > copy?
> >
> > Right, copy Should I simply remove the complete header?
>
> You could do. I don't think there's much in that test that's novel or worth
> asserting copyright over - but if you disagree and want to assign whatever
> is copyrightable to the FSF, keep the header but fix the years. Either way
> is fine by me.
>
> OK for trunk and backports, with the comments above suitably resolved.
--
──
Dr. Matthias Kretz https://mattkretz.github.io
GSI Helmholtz Centre for Heavy Ion Research https://gsi.de
stdₓ::simd
──
diff --git a/libstdc++-v3/include/experimental/bits/simd.h b/libstdc++-v3/include/experimental/bits/simd.h
index 224153ffbaf..b0571ca26c4 100644
--- a/libstdc++-v3/include/experimental/bits/simd.h
+++ b/libstdc++-v3/include/experimental/bits/simd.h
@@ -2675,7 +2675,14 @@ _SimdWrapper(_V __x)
_GLIBCXX_SIMD_INTRINSIC constexpr void
_M_set(size_t __i, _Tp __x)
-{ _M_data[__i] = __x; }
+{
+ if (__builtin_is_constant_evaluated())
+ _M_data = __generate_from_n_evaluations<_Width, _BuiltinType>([&](auto __j) {
+ return __j == __i ? __x : _M_data[__j()];
+ });
+ else
+ _M_data[__i] = __x;
+}
_GLIBCXX_SIMD_INTRINSIC
constexpr bool
@@ -3186,6 +3193,10 @@ resizing_simd_cast(const simd<_Up, _Ap>& __x)
{
if constexpr (is_same_v)
return __x;
+else if (__builtin_is_constant_evaluated())
+ return _Tp([&](auto __i) constexpr {
+ return __i < simd_size_v<_Up, _Ap> ? __x[__i] : _Up();
+ });
else if constexpr (simd_size_v<_Up, _Ap> == 1)
{
_Tp __r{};
@@ -3321,10 +3332,11 @@ __get_lvalue(const const_where_expression& __x)
const_where_expression& operator=(const const_where_expression&) = delete;
-_GLIBCXX_SIMD_INTRINSIC const_where_expression(const _M& __kk, const _Tp& dd)
- : _M_k(__kk), _M_value(const_cast<_Tp&>(dd)) {}
+_GLIBCXX_SIMD_INTRINSIC constexpr
+const_where_expression(const _M& __kk, const _Tp& dd)
+: _M_k(__kk), _M_value(const_cast<_Tp&>(dd)) {}
-_GLIBCXX_SIMD_INTRINSIC _V
+_GLIBCXX_SIMD_INTRINSIC _GLIBCXX_SIMD_CONSTEXPR _V
operator-() const&&
{
return {__private_init,
@@ -,7 +3345,7 @@ __get_lvalue(const const_where_expression& __x)
}
template
- [[nodiscard]] _GLIBCXX_SIMD_INTRINSIC _V
+ [[nodiscard]] _GLIBCXX_SIMD_INTRINSIC
Re: [PATCH] libstdc++: Add missing constexpr to simd
On Mon, 22 May 2023, Jonathan Wakely via Libstdc++ wrote: * subscripting vector builtins is not allowed in constant expressions Is that just because nobody made it work (yet)? Yes. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101651 and others. * if the implementation would otherwise call SIMD intrinsics/builtins https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80517 and others. Makes sense to work around them for now. -- Marc Glisse
Re: [PATCH] libstdc++: Add missing constexpr to simd
On Mon, 22 May 2023 at 21:27, Matthias Kretz wrote: > On Monday, 22 May 2023 18:25:15 CEST Jonathan Wakely wrote: > > I note that using if (not __builtin_constant_evaluated()) will fail if > > compiled with -fno-operator-names, which is why we don't use 'not', > 'and', > > etc. elsewhere in libstdc++. I don't know if (or why) anybody uses that > > option though, so I don't think you need to hange anything in stdx::simd. > > Ah, I just recently convinced myself that "operator-names" are more > readable > (=> easier to maintain). I tend to agree, but every time I decide to start using them some testcases start to fail and I remember why we don't use them :-( > But OTOH a mix isn't necessarily better. I'm fine > with keeping it consistent. > > > > * subscripting vector builtins is not allowed in constant expressions > > > > Is that just because nobody made it work (yet)? > > That is a good question. I guess I should open a PR. > > > * if the implementation needs/uses memcpy > > > > > * if the implementation would otherwise call SIMD intrinsics/builtins > > > > The indentation looks off here and in the _M_set member function > following > > it: > > Yes. I had to put an #if between an else and an if. Looks like this: > > else > #ifdef _GLIBCXX_SIMD_USE_ALIASING_LOADS > if (not __builtin_is_constant_evaluated()) > return reinterpret_cast*>(this)[__i]; > else > #endif > if constexpr (__is_scalar_abi<_Abi0>()) > > Ah yes, so the if is indented two spaces from the else above it. What looks wrong to me is that the return is the at the same indentation as the if controlling it. > Should the `if` be aligned to the `else` instead? > How about moving the two else tokens? #ifdef _GLIBCXX_SIMD_USE_ALIASING_LOADS else if (not __builtin_is_constant_evaluated()) return reinterpret_cast*>(this)[__i]; #endif else if constexpr (__is_scalar_abi<_Abi0>()) I think that avoids the issue. > > > Are the copyright years on > > testsuite/experimental/simd/pr109261_constexpr_simd.cc correct, or just > > copy? > > Right, copy Should I simply remove the complete header? > > You could do. I don't think there's much in that test that's novel or worth asserting copyright over - but if you disagree and want to assign whatever is copyrightable to the FSF, keep the header but fix the years. Either way is fine by me. OK for trunk and backports, with the comments above suitably resolved.
Re: [PATCH] libstdc++: Add missing constexpr to simd
On Monday, 22 May 2023 18:25:15 CEST Jonathan Wakely wrote: > I note that using if (not __builtin_constant_evaluated()) will fail if > compiled with -fno-operator-names, which is why we don't use 'not', 'and', > etc. elsewhere in libstdc++. I don't know if (or why) anybody uses that > option though, so I don't think you need to hange anything in stdx::simd. Ah, I just recently convinced myself that "operator-names" are more readable (=> easier to maintain). But OTOH a mix isn't necessarily better. I'm fine with keeping it consistent. > > * subscripting vector builtins is not allowed in constant expressions > > Is that just because nobody made it work (yet)? That is a good question. I guess I should open a PR. > * if the implementation needs/uses memcpy > > > * if the implementation would otherwise call SIMD intrinsics/builtins > > The indentation looks off here and in the _M_set member function following > it: Yes. I had to put an #if between an else and an if. Looks like this: else #ifdef _GLIBCXX_SIMD_USE_ALIASING_LOADS if (not __builtin_is_constant_evaluated()) return reinterpret_cast*>(this)[__i]; else #endif if constexpr (__is_scalar_abi<_Abi0>()) Should the `if` be aligned to the `else` instead? > Are the copyright years on > testsuite/experimental/simd/pr109261_constexpr_simd.cc correct, or just > copy? Right, copy Should I simply remove the complete header? - Matthias -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de stdₓ::simd ──
Re: [PATCH] libstdc++: Add missing constexpr to simd
On Mon, 22 May 2023 at 16:36, Matthias Kretz via Libstdc++ <
libstd...@gcc.gnu.org> wrote:
> OK for trunk and backporting?
>
> regtested on x86_64-linux and aarch64-linux
>
> The constexpr API is only available with -std=gnu++XX (and proposed for
> C++26). The proposal is to have the complete simd API usable in constant
> expressions.
>
> This patch resolves several issues with using simd in constant
> expressions.
>
> Issues why constant_evaluated branches are necessary:
>
I note that using if (not __builtin_constant_evaluated()) will fail if
compiled with -fno-operator-names, which is why we don't use 'not', 'and',
etc. elsewhere in libstdc++. I don't know if (or why) anybody uses that
option though, so I don't think you need to hange anything in stdx::simd.
> * subscripting vector builtins is not allowed in constant expressions
>
Is that just because nobody made it work (yet)?
* if the implementation needs/uses memcpy
> * if the implementation would otherwise call SIMD intrinsics/builtins
>
The indentation looks off here and in the _M_set member function following
it:
operator[](size_t __i) const noexcept
{
if constexpr (_S_tuple_size == 1)
return _M_subscript_read(__i);
else
- {
#ifdef _GLIBCXX_SIMD_USE_ALIASING_LOADS
- return reinterpret_cast*>(this)[__i];
-#else
- if constexpr (__is_scalar_abi<_Abi0>())
-{
- const _Tp* ptr =
- return ptr[__i];
-}
- else
-return __i < simd_size_v<_Tp, _Abi0>
- ? _M_subscript_read(__i)
- : second[__i - simd_size_v<_Tp, _Abi0>];
+ if (not __builtin_is_constant_evaluated())
+ return reinterpret_cast*>(this)[__i];
+ else
#endif
+ if constexpr (__is_scalar_abi<_Abi0>())
+ {
+ const _Tp* ptr =
+ return ptr[__i];
}
+ else
+ return __i < simd_size_v<_Tp, _Abi0> ? _M_subscript_read(__i)
+ : second[__i - simd_size_v<_Tp, _Abi0>];
}
Are the copyright years on
testsuite/experimental/simd/pr109261_constexpr_simd.cc correct, or just
copy?
[PATCH] libstdc++: Add missing constexpr to simd
OK for trunk and backporting?
regtested on x86_64-linux and aarch64-linux
The constexpr API is only available with -std=gnu++XX (and proposed for
C++26). The proposal is to have the complete simd API usable in constant
expressions.
This patch resolves several issues with using simd in constant
expressions.
Issues why constant_evaluated branches are necessary:
* subscripting vector builtins is not allowed in constant expressions
* if the implementation needs/uses memcpy
* if the implementation would otherwise call SIMD intrinsics/builtins
Signed-off-by: Matthias Kretz
libstdc++-v3/ChangeLog:
PR libstdc++/109261
* include/experimental/bits/simd.h (_SimdWrapper::_M_set):
Avoid vector builtin subscripting in constant expressions.
(resizing_simd_cast): Avoid memcpy if constant_evaluated.
(const_where_expression, where_expression, where)
(__extract_part, simd_mask, _SimdIntOperators, simd): Add either
_GLIBCXX_SIMD_CONSTEXPR (on public APIs), or constexpr (on
internal APIs).
* include/experimental/bits/simd_builtin.h (__vector_permute)
(__vector_shuffle, __extract_part, _GnuTraits::_SimdCastType1)
(_GnuTraits::_SimdCastType2, _SimdImplBuiltin)
(_MaskImplBuiltin::_S_store): Add constexpr.
(_CommonImplBuiltin::_S_store_bool_array)
(_SimdImplBuiltin::_S_load, _SimdImplBuiltin::_S_store)
(_SimdImplBuiltin::_S_reduce, _MaskImplBuiltin::_S_load): Add
constant_evaluated case.
* include/experimental/bits/simd_fixed_size.h
(_S_masked_load): Reword comment.
(__tuple_element_meta, __make_meta, _SimdTuple::_M_apply_r)
(_SimdTuple::_M_subscript_read, _SimdTuple::_M_subscript_write)
(__make_simd_tuple, __optimize_simd_tuple, __extract_part)
(__autocvt_to_simd, _Fixed::__traits::_SimdBase)
(_Fixed::__traits::_SimdCastType, _SimdImplFixedSize): Add
constexpr.
(_SimdTuple::operator[], _M_set): Add constexpr and add
constant_evaluated case.
(_MaskImplFixedSize::_S_load): Add constant_evaluated case.
* include/experimental/bits/simd_scalar.h: Add constexpr.
* include/experimental/bits/simd_x86.h (_CommonImplX86): Add
constexpr and add constant_evaluated case.
(_SimdImplX86::_S_equal_to, _S_not_equal_to, _S_less)
(_S_less_equal): Value-initialize to satisfy constexpr
evaluation.
(_MaskImplX86::_S_load): Add constant_evaluated case.
(_MaskImplX86::_S_store): Add constexpr and constant_evaluated
case. Value-initialize local variables.
(_MaskImplX86::_S_logical_and, _S_logical_or, _S_bit_not)
(_S_bit_and, _S_bit_or, _S_bit_xor): Add constant_evaluated
case.
* testsuite/experimental/simd/pr109261_constexpr_simd.cc: New
test.
---
libstdc++-v3/include/experimental/bits/simd.h | 153 ---
.../include/experimental/bits/simd_builtin.h | 100 ++
.../experimental/bits/simd_fixed_size.h | 177 +-
.../include/experimental/bits/simd_scalar.h | 78
.../include/experimental/bits/simd_x86.h | 68 +--
.../simd/pr109261_constexpr_simd.cc | 109 +++
6 files changed, 437 insertions(+), 248 deletions(-)
create mode 100644 libstdc++-v3/testsuite/experimental/simd/
pr109261_constexpr_simd.cc
--
──
Dr. Matthias Kretz https://mattkretz.github.io
GSI Helmholtz Centre for Heavy Ion Research https://gsi.de
stdₓ::simd
──diff --git a/libstdc++-v3/include/experimental/bits/simd.h b/libstdc++-v3/include/experimental/bits/simd.h
index 224153ffbaf..b0571ca26c4 100644
--- a/libstdc++-v3/include/experimental/bits/simd.h
+++ b/libstdc++-v3/include/experimental/bits/simd.h
@@ -2675,7 +2675,14 @@ _SimdWrapper(_V __x)
_GLIBCXX_SIMD_INTRINSIC constexpr void
_M_set(size_t __i, _Tp __x)
-{ _M_data[__i] = __x; }
+{
+ if (__builtin_is_constant_evaluated())
+ _M_data = __generate_from_n_evaluations<_Width, _BuiltinType>([&](auto __j) {
+ return __j == __i ? __x : _M_data[__j()];
+ });
+ else
+ _M_data[__i] = __x;
+}
_GLIBCXX_SIMD_INTRINSIC
constexpr bool
@@ -3186,6 +3193,10 @@ resizing_simd_cast(const simd<_Up, _Ap>& __x)
{
if constexpr (is_same_v)
return __x;
+else if (__builtin_is_constant_evaluated())
+ return _Tp([&](auto __i) constexpr {
+ return __i < simd_size_v<_Up, _Ap> ? __x[__i] : _Up();
+ });
else if constexpr (simd_size_v<_Up, _Ap> == 1)
{
_Tp __r{};
@@ -3321,10 +3332,11 @@ __get_lvalue(const const_where_expression& __x)
const_where_expression& operator=(const const_where_expression&) = delete;
-_GLIBCXX_SIMD_INTRINSIC const_where_expression(const _M&
