Bug 86276 - Poor codegen when returning a std::vector
Summary: Poor codegen when returning a std::vector
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: alias, missed-optimization
Depends on:
Blocks: std::vector
  Show dependency treegraph
 
Reported: 2018-06-22 01:43 UTC by Mathias Stearn
Modified: 2024-12-27 16:15 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-06-22 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Stearn 2018-06-22 01:43:44 UTC
https://godbolt.org/g/aCiuAy

While I'm a bit sad that good() isn't just "ret", I think that the current rules for allocation elision require something like that codegen.

I'd expect bad() to codegen to roughly the same as good(), but then rather than calling operator delete, it should store [rax, rax + 1, rax + 22] to the three qwords starting at the out pointer. Instead, it does things like checking if the vector pointer it just zeroed is still zero to see if it needs to be deleted. It also seems to register an exception landing pad (I'm guessing to cover the operator new call) to handle freeing the vector's memory, even though it should know it isn't holding any. 

If I had to guess, I'd say the problem is that it thinks the hidden return out pointer has escaped when it hasn't really until a successful return. I'm pretty sure nothing in the language allows any way to access the return value before a function returns like that, but I'm now really curious if I'm wrong. If there is, is there any way to tell gcc that I promise I'm not doing anything quite that stupid?

PS- is it helpful to include the code and asm here in addition to the godbolt links?

#include <vector>
#include <cstdint>

auto good() {
std::vector<uint8_t> something;
something.reserve(22);
something = {0x02};
//return something; Only difference from bad
}


auto bad() {
std::vector<uint8_t> something;
something.reserve(22);
something = {0x02};
return something;
}

good():
        sub     rsp, 8
        mov     edi, 22
        call    operator new(unsigned long)
        mov     BYTE PTR [rax], 2
        mov     rdi, rax
        add     rsp, 8
        jmp     operator delete(void*)
bad():
        push    rbp
        pxor    xmm0, xmm0
        push    rbx
        mov     rbx, rdi
        sub     rsp, 24
        mov     QWORD PTR [rdi+16], 0
        movups  XMMWORD PTR [rdi], xmm0
        mov     edi, 22
        call    operator new(unsigned long)
        mov     rdi, QWORD PTR [rbx]
        test    rdi, rdi
        je      .L5
        mov     QWORD PTR [rsp+8], rax
        call    operator delete(void*)
        mov     rax, QWORD PTR [rsp+8]
.L5:
        mov     BYTE PTR [rax], 2
        lea     rdx, [rax+22]
        mov     QWORD PTR [rbx], rax
        add     rax, 1
        mov     QWORD PTR [rbx+8], rax
        mov     rax, rbx
        mov     QWORD PTR [rbx+16], rdx
        add     rsp, 24
        pop     rbx
        pop     rbp
        ret
        mov     rbp, rax
        jmp     .L6
bad() [clone .cold.25]:
.L6:
        mov     rdi, QWORD PTR [rbx]
        test    rdi, rdi
        je      .L7
        call    operator delete(void*)
.L7:
        mov     rdi, rbp
        call    _Unwind_Resume
Comment 1 Richard Biener 2018-06-22 08:07:52 UTC
  <bb 2> [local count: 1073741825]:
  MEM[(struct _Vector_impl *)something_3(D)]._M_start = 0B;
  MEM[(struct _Vector_impl *)something_3(D)]._M_finish = 0B;
  MEM[(struct _Vector_impl *)something_3(D)]._M_end_of_storage = 0B;
  _26 = operator new (22);

  <bb 3> [local count: 354334802]:
  _23 = something_3(D)->D.15703._M_impl._M_start;
  if (_23 != 0B)


Confirmed.  So the reason we do not elide the NULL check is that GCC
thinks that the call to operator new may clobber the return slot
(well, generally clobber global memory).

For malloc() we assume it cannot but IIRC operator new () may be
overridden by the user and we could have (parts of) its implementation visible
to GCC which means we do have to assume it reads/clobbers global memory.

Note that we know that something_3(D) points to "local" memory but as you
say we see the pointer is returned and thus since points-to analysis is
flow-insensitive we have to assume it is already escaped at the call to
operator new.

I suppose we could handle returns specially but we have to beware of recursion
here and cannot handle it specially for IPA points-to.

So ... better points-to analysis to the rescue... (or somehow special-casing
operator new similar to malloc)
Comment 2 Jan Hubicka 2024-12-27 16:15:40 UTC
With -O3 we now do quite well.

_Z4goodv:
.LFB1248:
        .cfi_startproc
        ret
        .cfi_endproc
.LFE1248:
        .size   _Z4goodv, .-_Z4goodv
        .p2align 4
        .globl  _Z3badv
        .type   _Z3badv, @function
_Z3badv:
.LFB1260:
        .cfi_startproc
        pushq   %rbx
        .cfi_def_cfa_offset 16
        .cfi_offset 3, -16
        movq    %rdi, %rbx
        movl    $22, %edi
        call    _Znwm
        movb    $2, (%rax)
        leaq    22(%rax), %rdx
        movq    %rax, (%rbx)
        addq    $1, %rax
        movq    %rax, 8(%rbx)
        movq    %rbx, %rax
        movq    %rdx, 16(%rbx)
        popq    %rbx
        .cfi_def_cfa_offset 8
        ret

good is optimized to empty function since we now have __builtin_operator_new.
Bad is pretty straighforward construction of the vector:

struct vector bad ()
{
  unsigned char * __result;
  unsigned char * _26;
  unsigned char * _28;

  <bb 2> [local count: 1073741824]:
  _26 = operator new (22);
  something_3(D)->D.25721._M_impl.D.25032._M_start = _26;
  _28 = _26 + 22;
  something_3(D)->D.25721._M_impl.D.25032._M_end_of_storage = _28;
  MEM[(char * {ref-all})_26] = 2;
  __result_59 = _26 + 1;
  something_3(D)->D.25721._M_impl.D.25032._M_finish = __result_59;
  return something_3(D);

}
With -O2 however we end up offlining:

;; Function bad (_Z3badv, funcdef_no=1260, decl_uid=25847, cgraph_uid=171, symbol_order=187)

struct vector bad ()
{
  void * D.27512;
  const unsigned char D.25850[1];
  unsigned char * _33;
  unsigned char * _38;
  unsigned char * _40;
  unsigned char * _41;
  long int _42;
  long unsigned int _43;
  void * _44;
  long unsigned int _47;
  vector(2) long unsigned int _56;

  <bb 2> [local count: 1073741824]:
  MEM[(struct _Vector_impl_data *)something_3(D)]._M_start = 0B;
  MEM[(struct _Vector_impl_data *)something_3(D)]._M_end_of_storage = 0B;
  _38 = operator new (22);

  <bb 3> [local count: 1073741824]:
  _47 = (long unsigned int) _38;
  _56 = {_47, _47};
  MEM <vector(2) long unsigned int> [(unsigned char * *)something_3(D)] = _56;
  _33 = _38 + 22;
  something_3(D)->D.25721._M_impl.D.25032._M_end_of_storage = _33;
  D.25850[0] = 2;
  std::vector<unsigned char>::_M_assign_aux.isra (something_3(D), &D.25850, &MEM <const unsigned char[1]> [(void *)&D.25850 + 1B]);

  <bb 4> [local count: 1073741824]:
  D.25850 ={v} {CLOBBER(eos)};
  return something_3(D);

  <bb 5> [count: 0]:
<L2>:
  D.25850 ={v} {CLOBBER(eos)};
  _41 = MEM[(struct _Vector_base *)something_3(D)]._M_impl.D.25032._M_start;
  if (_41 != 0B)
    goto <bb 6>; [53.47%]
  else
    goto <bb 7>; [46.53%]

  <bb 6> [count: 0]:
  _40 = MEM[(struct _Vector_base *)something_3(D)]._M_impl.D.25032._M_end_of_storage;
  _42 = _40 - _41;
  _43 = (long unsigned int) _42;
  operator delete (_41, _43);

  <bb 7> [count: 0]:
  _44 = __builtin_eh_pointer (1);
  __builtin_unwind_resume (_44);

}

So it first constructs empty vector and then offlines assignment froom C array. Since it may throw an exception there is EH calling delete which is always noop.
I think we need to start with ability to IPA propagate that the vector is empty and thus _M_start==_M_end_of_storage

Curiously codegen for good is even longer:
void good () 
{ 
  void * D.27490; 
  unsigned char * something$_M_end_of_storage;
  unsigned char * something$_M_start;
  struct vector something;
  const unsigned char D.25837[1];
  void * _10;
  unsigned char * _36;
  unsigned char * _41;
  long int _45;
  long unsigned int _46;
  long int _49;
  long unsigned int _50;
  vector(2) long unsigned int _53;
  long unsigned int _59;

  <bb 2> [local count: 1073741824]:
  MEM[(struct _Vector_impl_data *)&something] ={v} {CLOBBER(bob)};
  _41 = operator new (22);
  goto <bb 4>; [100.00%]

  <bb 3> [count: 0]:
<L4>:
  D.25837 ={v} {CLOBBER(eos)};
  goto <bb 10>; [100.00%]

  <bb 4> [local count: 1073741824]:
  _59 = (long unsigned int) _41;
  _53 = {_59, _59};
  _36 = _41 + 22;
  D.25837[0] = 2;
  MEM <vector(2) long unsigned int> [(void *)&something] = _53;
  MEM <unsigned char *> [(struct vector *)&something + 16B] = _36;
  std::vector<unsigned char>::_M_assign_aux.isra (&something, &D.25837, &MEM <const unsigned char[1]> [(void *)&D.25837 + 1B]);

  <bb 5> [local count: 1073741824]:
  something$_M_start_20 = MEM <unsigned char *> [(struct vector *)&something];
  something$_M_end_of_storage_21 = MEM <unsigned char *> [(struct vector *)&something + 16B];
  D.25837 ={v} {CLOBBER(eos)};
  if (something$_M_start_20 != 0B)
    goto <bb 6>; [53.47%]
  else
    goto <bb 7>; [46.53%]

  <bb 6> [local count: 574129752]:
  _45 = something$_M_end_of_storage_21 - something$_M_start_20;
  _46 = (long unsigned int) _45;
  operator delete (something$_M_start_20, _46); [tail call]

  <bb 7> [local count: 1073741824]:
  something ={v} {CLOBBER(eob)};
  something ={v} {CLOBBER(eos)};
  return;

  <bb 8> [count: 0]:
<L3>:
  something$_M_start_22 = MEM <unsigned char *> [(struct vector *)&something];
  something$_M_end_of_storage_23 = MEM <unsigned char *> [(struct vector *)&something + 16B];
  D.25837 ={v} {CLOBBER(eos)};
  if (something$_M_start_22 != 0B)
    goto <bb 9>; [53.47%]
  else
    goto <bb 10>; [46.53%]

  <bb 9> [count: 0]:
  _49 = something$_M_end_of_storage_23 - something$_M_start_22;
  _50 = (long unsigned int) _49;
  operator delete (something$_M_start_22, _50);

  <bb 10> [count: 0]:
  something ={v} {CLOBBER(eob)};
  _10 = __builtin_eh_pointer (2);
  __builtin_unwind_resume (_10);

}