On Mon, 14 Jan 2019 at 08:29, Richard Biener <richard.guent...@gmail.com> wrote:
>
> On Thu, Jan 10, 2019 at 10:02 AM Jakub Jelinek <ja...@redhat.com> wrote:
> >
> > Hi!
> >
> > In Marc's testcase, we generate terrible code for std::string assignment,
> > because the __builtin_constant_p is kept in the IL for way too long and the
> > optimizers (jump threading?) create way too many copies of the
> > memcpy/memmove calls that it is then hard to bring it back in sanitity.
> > On the testcase in the PR, GCC 7 emits on x86_64 with -O2 99 bytes long
> > function, GCC 9 unpatched 259 bytes long, with this patch it emits
> > 139 bytes long, better but still not as good as before. I guess we'll need
> > to improve GIMPLE optimizers too, but having twice as small IL for these
> > heavily used operators where e.g. _M_disjunct uses two of them and we wind
> > up with twice as many branches because of that is IMHO very useful.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > 1) I'm not really sure about proper formatting in libstdc++, I thought you
> > don't use space before ( in function calls, but then why is there a space
> > in __builtin_constant_p?
> > 2) not really sure about that #if __cplusplus >= 201402L either, I think we
> > don't really want to use __builtin_is_constant_evaluated at least in
> > C++98 code, but even in C++11, if the operator isn't constexpr, is there
> > any point trying to help it do the right thing in constexpr contexts?
> >
> > 2019-01-09 Jakub Jelinek <ja...@redhat.com>
> >
> > PR tree-optimization/88775
> > * include/bits/stl_function.h (greater<_Tp*>::operator(),
> > less<_Tp*>::operator(), greater_equal<_Tp*>::operator(),
> > less_equal<_Tp*>::operator()): Use __builtin_is_constant_evaluated
> > instead of __builtin_constant_p if available. Don't bother with
> > the pointer comparison in C++11 and earlier.
> >
> > --- libstdc++-v3/include/bits/stl_function.h.jj 2019-01-01
> > 12:45:51.182541077 +0100
> > +++ libstdc++-v3/include/bits/stl_function.h 2019-01-09
> > 23:15:34.824800676 +0100
> > @@ -413,8 +413,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > _GLIBCXX14_CONSTEXPR bool
> > operator()(_Tp* __x, _Tp* __y) const _GLIBCXX_NOTHROW
> > {
> > +#if __cplusplus >= 201402L
> > +#ifdef _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
> > + if (__builtin_is_constant_evaluated())
> > +#else
> > if (__builtin_constant_p (__x > __y))
> > +#endif
> > return __x > __y;
> > +#endif
> > return (__UINTPTR_TYPE__)__x > (__UINTPTR_TYPE__)__y;
>
> I wonder what the idea behind this is. It smells like trying to avoid
> undefined behavior (relational compare of pointers to different objects?)
That's not undefined in C++, it just gives an unspecified result (so
it's not specified whether x < y or y < x, or possibly even x == y,
e.g. for segmented memory).
The std::greater, std::less etc. function objects are required to give
a total order across all pointers, different objects or not, i.e.
while < might give any unspecified result, std::less has tighter
restrictions.
For that to work in general, we need the casts, or GCC's optimizers
give the wrong result. But within constexpr those functions only need
to be valid for related objects (as in C) and so we just use < there,
and rely on the compiler to reject comparisons to different objects
(because that's not allowed in constexpr).
So we can't just use < everywhere, because the optimizers don't like
it, and we can't use the cast everywhere, because that's not allowed
in constexpr.
