Re: Implement C++20 constexpr <algorithm>, , and <array>

[Jonathan Wakely <jwakely at redhat dot com>]

On 29/03/19 12:02 -0400, Ed Smith-Rowland wrote:
> On 3/29/19 9:23 AM, Jakub Jelinek wrote:
> > On Fri, Mar 29, 2019 at 09:10:26AM -0400, Ed Smith-Rowland via gcc-patches wrote:
> > > Greetings,
> > >
> > > This patch implements C++20 constexpr for <algorithm>, <uility>, <array>.
> > >
> > > It's a large patch but only affects C++20 and the volume is mostly test
> > > cases.
> > >
> > > This differs from the previous patch in actually testing constexpr :-\ and
> > > in the addition of wrappers for __builtin_memmove and __builtin_memcmp that
> > > supply constexpr branches if C++20 and is_constant_evaluated().
> > +    void*
> > +    __memmove(_Tp* __dst, const _Tp* __src, ptrdiff_t __num)
> > +    {
> > +#if __cplusplus > 201703L
> > +      if (is_constant_evaluated())
> > +       {
> > +         for(; __num > 0; --__num)
> > +           {
> > +             *__dst = *__src;
> > +             ++__src;
> > +             ++__dst;
> > +           }
> > +         return __dst;
> > +       }
> > +      else if (__num)
> > +#endif
> > +       return __builtin_memmove(__dst, __src, sizeof(_Tp) * abs(__num));
> > +      return __dst;
> > ..
> > 	  const ptrdiff_t _Num = __last - __first;
> >           if (_Num)
> > -           __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
> > +           __memmove(__result, __first, _Num);
> > ..
> > 	  const ptrdiff_t _Num = __last - __first;
> >           if (_Num)
> > -           __builtin_memmove(__result - _Num, __first, sizeof(_Tp) * _Num);
> > +           __memmove(__result - _Num, __first, _Num);
> >
> > Why the abs in there, that is something that wasn't previously there and
> > if the compiler doesn't figure out that __last >= __first, it would mean
> > larger emitted code for the non-constexpr case.  As memmove argument is
> > size_t, wouldn't it be better to make __num just size_t and remove this abs?
> > Also, wouldn't it be better to have on the other side the __num == 0
> > handling inside of __memmove, you already have it there for C++2a, but not
> > for older.  You could then drop the if (_Num) guards around __memmove.
>
> memmove needs to be able to work with __last < __first also.

Why?

> I was getting negative __num and when passed to __builtin_memmove 
> which takes size_t got blowups.
>
> I'm not sure why it worked before. __builtin_memmove will work with 
> __last < __first and sensible positive __num.
>
> When I tried to do what __builtin_memmove or ::memmove must do with 
> unsigned num I would need to branch on __last < __first

How can last < first be true for any algorithm?

> and copy backwards.  But pointer comparisons were getting caught as 
> non-constexpr.
>
> I'll look at the __num==0 (noop) testing.
>
> > Also, shouldn't the is_constant_evaluated() calls be guarded with
> > _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED ?  Without that it won't be
> > defined...
>
> I am trying for a C++20-only patch (hoping to get it in for 9) so I 
> used the library function and tested __cplusplus > 201703L.
>
> We could do _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED and then we 
> could use these for lower version. Maybe stage 1?

The reason to check _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED is
that std::is_constant_evaluated() is not available when compiling with
Clang, because it doesn't provide the built-in yet.

It's fine to make the algos not constexpr when the builtin isn't
available, it's not fine to just use something that won't compile.