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

[Matthias Kretz]

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


-- 
──────────────────────────────────────────────────────────────────────────
 Dr. Matthias Kretz                           https://mattkretz.github.io
 GSI Helmholtz Centre for Heavy Ion Research               https://gsi.de
 stdₓ::simd
──────────────────────────────────────────────────────────────────────────
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libstdc-Add-missing-constexpr-to-simd.patch
Type: text/x-patch
Size: 73521 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20230523/c85c0a6d/attachment-0001.bin>