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

["Ed Smith-Rowland via gcc-patches" <gcc-patches at gcc dot gnu dot org>]

On 3/29/19 12:39 PM, Jakub Jelinek wrote:
> On Fri, Mar 29, 2019 at 12:02:48PM -0400, Ed Smith-Rowland wrote:
> > > > 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.
> I don't get it, you are replacing calls with __builtin_memmove with
> __memmove, and the __builtin_memmove calls didn't do anything like that,
> the last argument is size_t and didn't use any abs.  So are you saying you
> see crashes with the current code (when not in constexpr contexts) that your
> patch fixes?
>
> 	Jakub
If std::copy is intended to work with first < last then yes. OTOH, the 
copy_move is just an impl detail for speed.

The std doesn't say that first >= last as a precondition but has 
sentences like:

"For each non-negative integer n < (last - first), performs *(result + 
n) = *(first + n)."

If this fixes a bug then I should make a pug report, a separate patch, 
and probably backport it.

I also took out the else if (__num) in the __memmove since this check is 
done at both call sites.

I made __memmove and __memcmp inline so that, certainly for C++ < 20 
these don't pessimize.

Retesting.

Ed


int
main()
{
  float arr[1000];
  float brr[1000];
  std::copy(arr + 500, arr, brr + 500);
}