Bug 109205 - vector.resize( v.size() + 100 ) does unnecessary comparison
Summary: vector.resize( v.size() + 100 ) does unnecessary comparison
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 12.2.0
: P3 enhancement
Target Milestone: 15.0
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on:
Blocks: std::vector
  Show dependency treegraph
 
Reported: 2023-03-20 08:26 UTC by Jörg Richter
Modified: 2024-12-21 08:01 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2023-03-20 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jörg Richter 2023-03-20 08:26:47 UTC
This function:

#include <vector>

void testResize( std::vector<char> & v )
{
  v.resize( v.size() + 100 );
}


Will compile (-O2) to something like this:

        mov     rcx, QWORD PTR [rdi+8]
        mov     rax, QWORD PTR [rdi]
        mov     rdx, rcx
        sub     rdx, rax
        lea     rsi, [rdx+100]
        cmp     rdx, rsi
        jb      .L39
        add     rax, rsi
        cmp     rcx, rax
        je      .L36
        mov     QWORD PTR [rdi+8], rax
.L36:
        ret
.L39:
        mov     esi, 100
        jmp     std::vector<char, std::allocator<char> >::_M_default_append(unsigned long)

The call to _M_default_append is guarded by a cmp.  This seems unnecessary, as the argument passed to resize is always bigger than size.  

Doing the addition with a signed type does not change the result.

See: https://godbolt.org/z/xY77fsz4z
Comment 1 Drea Pinski 2023-03-20 18:09:27 UTC
So the IR looks like:
  _4 = MEM[(const struct vector *)v_3(D)].D.25711._M_impl.D.25018._M_finish;
  _6 = MEM[(const struct vector *)v_3(D)].D.25711._M_impl.D.25018._M_start;
  _7 = _4 - _6;
  _8 = (long unsigned int) _7;
  _1 = _8 + 100;

GCC does not know this statement holds true:
  if (v.end() < v.begin()) __builtin_unreachable();

If you add that, then it will optimize away the comparison.
Comment 2 Jonathan Wakely 2023-03-20 22:01:20 UTC
I guess we might as well do it for capacity too:

--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -985,7 +985,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
       size_type
       size() const _GLIBCXX_NOEXCEPT
-      { return size_type(this->_M_impl._M_finish - this->_M_impl._M_start); }
+      {
+       if (this->_M_impl._M_finish < this->_M_impl._M_start)
+         __builtin_unreachable();
+       return size_type(this->_M_impl._M_finish - this->_M_impl._M_start);
+      }
 
       /**  Returns the size() of the largest possible %vector.  */
       _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
@@ -1071,8 +1075,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
       size_type
       capacity() const _GLIBCXX_NOEXCEPT
-      { return size_type(this->_M_impl._M_end_of_storage
-                        - this->_M_impl._M_start); }
+      {
+       if (this->_M_impl._M_end_of_storage < this->_M_impl._M_start)
+         __builtin_unreachable();
+       return size_type(this->_M_impl._M_end_of_storage
+                        - this->_M_impl._M_start);
+      }
 
       /**
        *  Returns true if the %vector is empty.  (Thus begin() would
Comment 3 Jonathan Wakely 2023-03-21 00:35:01 UTC
Huh, but that causes a test to FAIL with -D_GLIBCXX_DEBUG

FAIL: 23_containers/vector/59829.cc (test for excess errors)


/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/stl_vector.h:989: error: no match for 'operator<' in '((const std::__cxx1998::vector<int, Alloc<int> >*)this)->std::__cxx1998::vector<int, Alloc<int> >::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int> >::_M_impl.std::__cxx1998::_Vector_base<int, Alloc<int> >::_Vector_impl::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int> >::_Vector_impl_data::_M_finish < ((const std::__cxx1998::vector<int, Alloc<int> >*)this)->std::__cxx1998::vector<int, Alloc<int> >::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int> >::_M_impl.std::__cxx1998::_Vector_base<int, Alloc<int> >::_Vector_impl::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int> >::_Vector_impl_data::_M_start' (operand types are 'const std::__cxx1998::_Vector_base<int, Alloc<int> >::pointer' {aka 'const std::allocator_traits<Alloc<int> >::pointer'} and 'const std::__cxx1998::_Vector_base<int, Alloc<int> >::pointer' {aka 'const std::allocator_traits<Alloc<int> >::pointer'})
Comment 4 Jonathan Wakely 2023-06-01 10:53:59 UTC
I added hints to size() and capacity() and it caused regressions, see PR 110060

It makes it less likely for size() to be inlined, and causes:

FAIL: g++.dg/pr104547.C  -std=gnu++14  scan-tree-dump-not vrp2 "_M_default_append"
Comment 5 Jonathan Wakely 2023-06-01 10:56:22 UTC
(In reply to Jonathan Wakely from comment #3)
> Huh, but that causes a test to FAIL with -D_GLIBCXX_DEBUG
> 
> FAIL: 23_containers/vector/59829.cc (test for excess errors)
> 
> 
> /home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/
> stl_vector.h:989: error: no match for 'operator<' in '((const
> std::__cxx1998::vector<int, Alloc<int> >*)this)->std::__cxx1998::vector<int,
> Alloc<int> >::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int>
> >::_M_impl.std::__cxx1998::_Vector_base<int, Alloc<int>
> >::_Vector_impl::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int>
> >::_Vector_impl_data::_M_finish < ((const std::__cxx1998::vector<int,
> Alloc<int> >*)this)->std::__cxx1998::vector<int, Alloc<int>
> >::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int>
> >::_M_impl.std::__cxx1998::_Vector_base<int, Alloc<int>
> >::_Vector_impl::<anonymous>.std::__cxx1998::_Vector_base<int, Alloc<int>
> >::_Vector_impl_data::_M_start' (operand types are 'const
> std::__cxx1998::_Vector_base<int, Alloc<int> >::pointer' {aka 'const
> std::allocator_traits<Alloc<int> >::pointer'} and 'const
> std::__cxx1998::_Vector_base<int, Alloc<int> >::pointer' {aka 'const
> std::allocator_traits<Alloc<int> >::pointer'})

This was fixed by r14-1249-g8d2fa90a415676
Comment 6 Drea Pinski 2024-12-21 07:58:29 UTC
Fixed by r15-5361-gaac5c57ee16723 .
Comment 7 Drea Pinski 2024-12-21 08:01:11 UTC
(In reply to Jonathan Wakely from comment #4)
> I added hints to size() and capacity() and it caused regressions, see PR
> 110060
> 
> It makes it less likely for size() to be inlined, and causes:
> 
> FAIL: g++.dg/pr104547.C  -std=gnu++14  scan-tree-dump-not vrp2
> "_M_default_append"

The inlining issue was fixed with r15-5336-gcee7d080d5c2a5 (and a few others).