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?) but then executing that nevertheless when "constant"? I think this just doesn't work since the compiler, when evaluating __x > __y [for constant folding] is exploiting the fact that doing non-equality compares on pointers into different objects invoke undefined behavior. So why is this not just return (__UINTPTR_TYPE__)__x > (__UINTPTR_TYPE__)__y; or with the casts elided? Does the C++ standard say pointers are to be compared unsigned here? Or do all targets GCC support lay out the address space in a way that this is correct for pointers into distinct objects? Richard. > } > }; > @@ -426,8 +432,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; > } > }; > @@ -439,8 +451,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; > } > }; > @@ -452,8 +470,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; > } > }; > > Jakub