Re: [PATCH] libstdc++: Add missing constexpr to simd

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]

On Mon, 22 May 2023 at 21:27, Matthias Kretz <m.kretz@gsi.de> 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<const __may_alias<_Tp>*>(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<const __may_alias<_Tp>*>(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&paste?
>
> Right, copy&paste. 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.