On Tue, 26 Nov 2024 at 15:43, Jan Hubicka <[email protected]> wrote:
>
> Hi,
> here is updated patch.
>
> I am not ceratin if:
> const size_t __diffmax
> = __gnu_cxx::__numeric_traits<ptrdiff_t>::__max / sizeof(_CharT);
> really needs "/ sizeof (_CharT)". I think we only need to be able to
> compute the difference between two entries in multiplies of sizeof(_CharT)?
> However it is consistent with what std::vector does. Do we really need
> to be also able to represent differences in number of bytes?
>
> I was also thinking about API change implication. I hope that indeed
> real code does not need knowing max size and it is mostly needed to let
> GCC to VRP optimize the memory allocations.
>
> I also noticed that this patch trigger empty-loop.C failure which I
> originaly attributed to different change. I filled PR117764 on that.
> We are no longer able to eliminate empty loops early, but we still
> optimize them late.
>
> I also changed empty() to do the test directly (as done by std::vector
> and friends) instead of computing size. I do not think having
> __builtin_unreachable range check is useful there, so this should save
> some compile-time effort.
Yes, that makes sense.
> Bootstrapped/regtested x86_64-linux, OK?
Looks good, OK for trunk, thanks.
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/basic_string.h (basic_string::size(),
> basic_string::length(), basic_string::capacity()): Add
> __builtin_unreachable to declare value ranges.
> (basic_string::empty): Inline test.
> (basic_string::max_size()): Account correctly the terminating 0
> and limits implied by ptrdiff_t.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/tree-ssa/empty-loop.C: xfail optimization at cddce2 and check
> it happens at cddce3.
> * g++.dg/tree-ssa/string-1.C: New test.
>
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/empty-loop.C
> b/gcc/testsuite/g++.dg/tree-ssa/empty-loop.C
> index ed4a603bf5b..b7e7e27cc04 100644
> --- a/gcc/testsuite/g++.dg/tree-ssa/empty-loop.C
> +++ b/gcc/testsuite/g++.dg/tree-ssa/empty-loop.C
> @@ -30,5 +30,8 @@ int foo (vector<string> &v, list<string> &l, set<string>
> &s, map<int, string> &m
>
> return 0;
> }
> -/* { dg-final { scan-tree-dump-not "if" "cddce2"} } */
> +/* Adding __builtin_unreachable to std::string::size() prevents cddce2 from
> + eliminating the loop early, see PR117764. */
> +/* { dg-final { scan-tree-dump-not "if" "cddce2" { xfail *-*-* } } } */
> +/* { dg-final { scan-tree-dump-not "if" "cddce3"} } */
>
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/string-1.C
> b/gcc/testsuite/g++.dg/tree-ssa/string-1.C
> new file mode 100644
> index 00000000000..d38c23a7628
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/string-1.C
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -std=c++20 -fdump-tree-optimized" } */
> +#include <string>
> +std::string
> +test (std::string &a)
> +{
> + return a;
> +}
> +/* { dg-final { scan-tree-dump-not "throw" "optimized" } } */
> diff --git a/libstdc++-v3/include/bits/basic_string.h
> b/libstdc++-v3/include/bits/basic_string.h
> index f5b320099b1..17b973c8b45 100644
> --- a/libstdc++-v3/include/bits/basic_string.h
> +++ b/libstdc++-v3/include/bits/basic_string.h
> @@ -1079,20 +1079,30 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> size_type
> size() const _GLIBCXX_NOEXCEPT
> - { return _M_string_length; }
> + {
> + size_type __sz = _M_string_length;
> + if (__sz > max_size ())
> + __builtin_unreachable ();
> + return __sz;
> + }
>
> /// Returns the number of characters in the string, not including any
> /// null-termination.
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> size_type
> length() const _GLIBCXX_NOEXCEPT
> - { return _M_string_length; }
> + { return size(); }
>
> /// Returns the size() of the largest possible %string.
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> size_type
> max_size() const _GLIBCXX_NOEXCEPT
> - { return (_Alloc_traits::max_size(_M_get_allocator()) - 1) / 2; }
> + {
> + const size_t __diffmax
> + = __gnu_cxx::__numeric_traits<ptrdiff_t>::__max / sizeof(_CharT);
> + const size_t __allocmax = _Alloc_traits::max_size(_M_get_allocator());
> + return (std::min)(__diffmax, __allocmax) - 1;
> + }
>
> /**
> * @brief Resizes the %string to the specified number of characters.
> @@ -1184,8 +1194,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> size_type
> capacity() const _GLIBCXX_NOEXCEPT
> {
> - return _M_is_local() ? size_type(_S_local_capacity)
> - : _M_allocated_capacity;
> + size_t __sz = _M_is_local() ? size_type(_S_local_capacity)
> + : _M_allocated_capacity;
> + if (__sz < _S_local_capacity || __sz > max_size ())
> + __builtin_unreachable ();
> + return __sz;
> }
>
> /**
> @@ -1234,7 +1247,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> bool
> empty() const _GLIBCXX_NOEXCEPT
> - { return this->size() == 0; }
> + { return _M_string_length == 0; }
>
> // Element access:
> /**
>
