Re: [PATCH] Use __builtin_is_constant_evaluated in std::less etc. (PR tree-optimization/88775)

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Jan 10, 2019 at 10:02 AM Jakub Jelinek <jakub@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  <jakub@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