public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function
@ 2023-04-06 19:00 hiraditya at msn dot com
  2023-04-11 13:28 ` [Bug tree-optimization/109442] " rguenth at gcc dot gnu.org
                   ` (20 more replies)
  0 siblings, 21 replies; 23+ messages in thread
From: hiraditya at msn dot com @ 2023-04-06 19:00 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

            Bug ID: 109442
           Summary: Dead local copy of std::vector not removed from
                    function
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hiraditya at msn dot com
  Target Milestone: ---

T vat1(std::vector<T> v1) {
    auto v = v1;
    return 10;
}

g++ -O3 -std=c++20 -fno-exceptions

vat1(std::vector<int, std::allocator<int> >):
        mov     rax, QWORD PTR [rdi+8]
        sub     rax, QWORD PTR [rdi]
        je      .L11
        push    rbp
        mov     rbp, rax
        movabs  rax, 9223372036854775804
        push    rbx
        sub     rsp, 8
        cmp     rax, rbp
        jb      .L15
        mov     rbx, rdi
        mov     rdi, rbp
        call    operator new(unsigned long)
        mov     rsi, QWORD PTR [rbx]
        mov     rdx, QWORD PTR [rbx+8]
        mov     rdi, rax
        sub     rdx, rsi
        cmp     rdx, 4
        jle     .L16
        call    memmove
        mov     rdi, rax
.L6:
        mov     rsi, rbp
        call    operator delete(void*, unsigned long)
        add     rsp, 8
        mov     eax, 10
        pop     rbx
        pop     rbp
        ret
.L11:
        mov     eax, 10
        ret
.L15:
        call    std::__throw_bad_array_new_length()
.L16:
        jne     .L6
        mov     eax, DWORD PTR [rsi]
        mov     DWORD PTR [rdi], eax
        jmp     .L6

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug tree-optimization/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
@ 2023-04-11 13:28 ` rguenth at gcc dot gnu.org
  2023-04-11 13:33 ` redi at gcc dot gnu.org
                   ` (19 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-04-11 13:28 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|                            |jwakely.gcc at gmail dot com,
                   |                            |rguenth at gcc dot gnu.org
   Last reconfirmed|                            |2023-04-11
            Version|unknown                     |13.0

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
So we're failing to DSE

  _13 = pretmp_25 - pretmp_63;
  if (_13 > 4)
    goto <bb 7>; [97.30%]
  else
    goto <bb 6>; [2.70%]

  <bb 6> [local count: 14278734]:
  if (_13 == 4)
    goto <bb 8>; [34.00%]
  else
    goto <bb 9>; [66.00%]

  <bb 7> [local count: 913536322]:
  _23 = (long unsigned int) _13;
  __builtin_memmove (_37, pretmp_63, _23);
  goto <bb 9>; [100.00%]

  <bb 8> [local count: 34511373]:
  _24 = *pretmp_63;
  *_37 = _24;

  <bb 9> [local count: 542742079]:
  operator delete (_37, _49);

because I think the DTOR / overloaded global delete might inspect the vector
contents.  So I'm not sure it would be valid to elide the memmove/store.
When the stores would be elided we'd DCE the new/delete pair as well.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug tree-optimization/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
  2023-04-11 13:28 ` [Bug tree-optimization/109442] " rguenth at gcc dot gnu.org
@ 2023-04-11 13:33 ` redi at gcc dot gnu.org
  2023-04-11 13:38 ` redi at gcc dot gnu.org
                   ` (18 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-11 13:33 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Neither v nor v1 escapes the function, so I don't think operator delete can
inspect them.

The destructor doesn't inspect the contents, it just destroys the elements
(which is a no-op for int) and then calls operator delete to free the storage.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug tree-optimization/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
  2023-04-11 13:28 ` [Bug tree-optimization/109442] " rguenth at gcc dot gnu.org
  2023-04-11 13:33 ` redi at gcc dot gnu.org
@ 2023-04-11 13:38 ` redi at gcc dot gnu.org
  2023-04-12  7:40 ` [Bug libstdc++/109442] " rguenth at gcc dot gnu.org
                   ` (17 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-11 13:38 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Ah, maybe the problem is that the library code manually elides destroying the
elements, precisely because it's a no-op. So we don't actually destroy the
elements, which means the compiler might think they're still initialized and so
could be inspected.

If the library explicitly does vec[i].~T() for every i then would that help?
The compiler would know there are no valid elements in the storage, and so
nothing operator delete could inspect.

We could continue to elide destroying the elements when !defined __OPTIMIZE__
so that we don't run a loop that does nothing, but with optimization enabled
rely on the compiler to remove that loop.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (2 preceding siblings ...)
  2023-04-11 13:38 ` redi at gcc dot gnu.org
@ 2023-04-12  7:40 ` rguenth at gcc dot gnu.org
  2023-04-12  9:29 ` redi at gcc dot gnu.org
                   ` (16 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-04-12  7:40 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |libstdc++

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #3)
> Ah, maybe the problem is that the library code manually elides destroying
> the elements, precisely because it's a no-op. So we don't actually destroy
> the elements, which means the compiler might think they're still initialized
> and so could be inspected.
> 
> If the library explicitly does vec[i].~T() for every i then would that help?
> The compiler would know there are no valid elements in the storage, and so
> nothing operator delete could inspect.
> 
> We could continue to elide destroying the elements when !defined
> __OPTIMIZE__ so that we don't run a loop that does nothing, but with
> optimization enabled rely on the compiler to remove that loop.

I don't think that would help.  The issue is the compiler thinks that

operator delete (_37, _49);

uses the memory at _37 and thus the stores

*_37 = _24;

and

__builtin_memmove (_37, pretmp_63, _23);

are not dead.  IIRC 'operator delete' (_ZdlPvm in this case), can be
overridden by the user and can inspect the memory state before "releasing"
the storage?

This also seems to be a form not handled by fndecl_dealloc_argno
even though it's marked as DECL_IS_OPERATOR_DELETE_P and
DECL_IS_REPLACEABLE_OPERATOR - but the actual call stmt is not marked
as such.  That's to catch a new/delete _expression_ and not a direct
call to the operator - ISTR we need the semantics guaranteed by the
standard for new/delete expressions here.

I see ~_Vector_base uses

 typedef __gnu_cxx::__alloc_traits<_Tp_alloc_type> _Tr;
 if (__p)
   _Tr::deallocate(_M_impl, __p, __n);

but I fail to trace that further (in the preprocessed source), the
line info on the delete stmt above points to new_allocator.h:168
which is

        _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));

which looks like a direct invocation of operator delete rather than
a delete expression.  So the compiler rightfully(?) refuses to apply
strict semantics ('delete' is _just_ like free with no other side-effects,
a 'new' / 'delete' pair may be elided).

Indeed in preprocessed source the above expands to

 ::operator delete((__p), (__n) * sizeof(_Tp));

rather than

  delete[] __p;

(or what the correct syntax with explicit size would be).  In theory
we could implement an attribute specifying a operator new or delete
invocation acts like a new or delete expression and use that in the
library and make sure that CALL_FROM_NEW_OR_DELETE_P is set on the
generated CALL_EXPRs.

When I replace the above operator invocation in the library with

  delete[] (char *)__p;

then the dead stores are elided but since I didn't track down the call
to 'operator new' which suffers from a similar problem the new/delete
pair isn't elided yet.

So in the end it seems this is a library/C++ frontend issue.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (3 preceding siblings ...)
  2023-04-12  7:40 ` [Bug libstdc++/109442] " rguenth at gcc dot gnu.org
@ 2023-04-12  9:29 ` redi at gcc dot gnu.org
  2023-04-12  9:45 ` rguenth at gcc dot gnu.org
                   ` (15 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-12  9:29 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> (In reply to Jonathan Wakely from comment #3)
> > Ah, maybe the problem is that the library code manually elides destroying
> > the elements, precisely because it's a no-op. So we don't actually destroy
> > the elements, which means the compiler might think they're still initialized
> > and so could be inspected.
> > 
> > If the library explicitly does vec[i].~T() for every i then would that help?
> > The compiler would know there are no valid elements in the storage, and so
> > nothing operator delete could inspect.
> > 
> > We could continue to elide destroying the elements when !defined
> > __OPTIMIZE__ so that we don't run a loop that does nothing, but with
> > optimization enabled rely on the compiler to remove that loop.
> 
> I don't think that would help.  The issue is the compiler thinks that
> 
> operator delete (_37, _49);
> 
> uses the memory at _37 and thus the stores
> 
> *_37 = _24;
> 
> and
> 
> __builtin_memmove (_37, pretmp_63, _23);
> 
> are not dead.

But if the library did _37->~_Tp() to destroy the element at _37 then it would
be dead, and accessing the element outside its lifetime would be undefined. The
bytes can only be accessed as char, unsigned char or std::byte after that.

> IIRC 'operator delete' (_ZdlPvm in this case), can be
> overridden by the user and can inspect the memory state before "releasing"
> the storage?

That would be insane for operator delete to do that. Maybe possible, but
insane.

In any case, the stores to _37 would be dead now, right? So even if operator
delete inspects the memory as raw bytes using memcmp or similar, it's reading
uninitialized storage, so any value is acceptable. So the stores to _37 should
be DSE-able now.

> This also seems to be a form not handled by fndecl_dealloc_argno
> even though it's marked as DECL_IS_OPERATOR_DELETE_P and
> DECL_IS_REPLACEABLE_OPERATOR - but the actual call stmt is not marked
> as such.  That's to catch a new/delete _expression_ and not a direct
> call to the operator - ISTR we need the semantics guaranteed by the
> standard for new/delete expressions here.
> 
> I see ~_Vector_base uses
> 
>  typedef __gnu_cxx::__alloc_traits<_Tp_alloc_type> _Tr;
>  if (__p)
>    _Tr::deallocate(_M_impl, __p, __n);
> 
> but I fail to trace that further (in the preprocessed source), the
> line info on the delete stmt above points to new_allocator.h:168
> which is
> 
>         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
> 
> which looks like a direct invocation of operator delete rather than
> a delete expression.  So the compiler rightfully(?) refuses to apply
> strict semantics ('delete' is _just_ like free with no other side-effects,
> a 'new' / 'delete' pair may be elided).
> 
> Indeed in preprocessed source the above expands to
> 
>  ::operator delete((__p), (__n) * sizeof(_Tp));
> 
> rather than
> 
>   delete[] __p;

Yes, that's correct. Using a delete expression here would be completely wrong.


> (or what the correct syntax with explicit size would be).  In theory
> we could implement an attribute specifying a operator new or delete
> invocation acts like a new or delete expression and use that in the
> library and make sure that CALL_FROM_NEW_OR_DELETE_P is set on the
> generated CALL_EXPRs.
> 
> When I replace the above operator invocation in the library with
> 
>   delete[] (char *)__p;

That would make std::vector incorrect though.

> then the dead stores are elided but since I didn't track down the call
> to 'operator new' which suffers from a similar problem the new/delete
> pair isn't elided yet.
> 
> So in the end it seems this is a library/C++ frontend issue.

Then there's no bug. std::vector is correct, and using a delete[] expression
would be a bug.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (4 preceding siblings ...)
  2023-04-12  9:29 ` redi at gcc dot gnu.org
@ 2023-04-12  9:45 ` rguenth at gcc dot gnu.org
  2023-04-12 10:01 ` redi at gcc dot gnu.org
                   ` (14 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-04-12  9:45 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
                 CC|                            |jason at gcc dot gnu.org
         Resolution|WONTFIX                     |---

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> (In reply to Richard Biener from comment #4)
> > (In reply to Jonathan Wakely from comment #3)
> > > Ah, maybe the problem is that the library code manually elides destroying
> > > the elements, precisely because it's a no-op. So we don't actually destroy
> > > the elements, which means the compiler might think they're still initialized
> > > and so could be inspected.
> > > 
> > > If the library explicitly does vec[i].~T() for every i then would that help?
> > > The compiler would know there are no valid elements in the storage, and so
> > > nothing operator delete could inspect.
> > > 
> > > We could continue to elide destroying the elements when !defined
> > > __OPTIMIZE__ so that we don't run a loop that does nothing, but with
> > > optimization enabled rely on the compiler to remove that loop.
> > 
> > I don't think that would help.  The issue is the compiler thinks that
> > 
> > operator delete (_37, _49);
> > 
> > uses the memory at _37 and thus the stores
> > 
> > *_37 = _24;
> > 
> > and
> > 
> > __builtin_memmove (_37, pretmp_63, _23);
> > 
> > are not dead.
> 
> But if the library did _37->~_Tp() to destroy the element at _37 then it
> would be dead, and accessing the element outside its lifetime would be
> undefined. The bytes can only be accessed as char, unsigned char or
> std::byte after that.
> 
> > IIRC 'operator delete' (_ZdlPvm in this case), can be
> > overridden by the user and can inspect the memory state before "releasing"
> > the storage?
> 
> That would be insane for operator delete to do that. Maybe possible, but
> insane.

Well, the standard doesn't prohibit "insanity" so we have to generate
correct code even in those cases, no?  After all this is what PR101480
was about ("insane" stuff) done even to new/delete _expressions_.

Would it be correct (by the standards wording) to extend the current
handling of new/delete expressions - which is that 'new' returns a pointer
not aliasing to any other pointer and that 'delete' does not read from
the memory pointed to by the first argument and this pointer also does not
escape to direct calls of operator new/delete?

Basically in gimple.cc:gimple_call_fnspec we currenty do

  /* If the call is to a replaceable operator delete and results
     from a delete expression as opposed to a direct call to
     such operator, then we can treat it as free.  */
  if (fndecl
      && DECL_IS_OPERATOR_DELETE_P (fndecl)
      && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
      && gimple_call_from_new_or_delete (stmt))
    return ". o ";
  /* Similarly operator new can be treated as malloc.  */
  if (fndecl
      && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
      && gimple_call_from_new_or_delete (stmt))
    return "m ";

where the comments do not exactly match the behavior (we also allow
arbitrary side-effects on global memory).  That we model 'delete'
as possibly writing to the released storage is so it serves as barrier for
earlier loads/stores to avoid sinking them across.

> In any case, the stores to _37 would be dead now, right? So even if operator
> delete inspects the memory as raw bytes using memcmp or similar, it's
> reading uninitialized storage, so any value is acceptable. So the stores to
> _37 should be DSE-able now.

There's noting in the IL indicating that though (but I haven't updated
libstd++ a while to create a updated preprocessed source - will do so now).

> > This also seems to be a form not handled by fndecl_dealloc_argno
> > even though it's marked as DECL_IS_OPERATOR_DELETE_P and
> > DECL_IS_REPLACEABLE_OPERATOR - but the actual call stmt is not marked
> > as such.  That's to catch a new/delete _expression_ and not a direct
> > call to the operator - ISTR we need the semantics guaranteed by the
> > standard for new/delete expressions here.
> > 
> > I see ~_Vector_base uses
> > 
> >  typedef __gnu_cxx::__alloc_traits<_Tp_alloc_type> _Tr;
> >  if (__p)
> >    _Tr::deallocate(_M_impl, __p, __n);
> > 
> > but I fail to trace that further (in the preprocessed source), the
> > line info on the delete stmt above points to new_allocator.h:168
> > which is
> > 
> >         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
> > 
> > which looks like a direct invocation of operator delete rather than
> > a delete expression.  So the compiler rightfully(?) refuses to apply
> > strict semantics ('delete' is _just_ like free with no other side-effects,
> > a 'new' / 'delete' pair may be elided).
> > 
> > Indeed in preprocessed source the above expands to
> > 
> >  ::operator delete((__p), (__n) * sizeof(_Tp));
> > 
> > rather than
> > 
> >   delete[] __p;
> 
> Yes, that's correct. Using a delete expression here would be completely
> wrong.

I see.  That's unfortunate (we'll never be able to elide the actual
allocation then).

> > (or what the correct syntax with explicit size would be).  In theory
> > we could implement an attribute specifying a operator new or delete
> > invocation acts like a new or delete expression and use that in the
> > library and make sure that CALL_FROM_NEW_OR_DELETE_P is set on the
> > generated CALL_EXPRs.
> > 
> > When I replace the above operator invocation in the library with
> > 
> >   delete[] (char *)__p;
> 
> That would make std::vector incorrect though.
> 
> > then the dead stores are elided but since I didn't track down the call
> > to 'operator new' which suffers from a similar problem the new/delete
> > pair isn't elided yet.
> > 
> > So in the end it seems this is a library/C++ frontend issue.
> 
> Then there's no bug. std::vector is correct, and using a delete[] expression
> would be a bug.

OK, then how do we arrange things so the compiler knows it can completely
elide the unused std::vector?  Or do you say it's not valid to do that?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (5 preceding siblings ...)
  2023-04-12  9:45 ` rguenth at gcc dot gnu.org
@ 2023-04-12 10:01 ` redi at gcc dot gnu.org
  2023-04-12 10:13 ` redi at gcc dot gnu.org
                   ` (13 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-12 10:01 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #6)
> (In reply to Jonathan Wakely from comment #5)
> > (In reply to Richard Biener from comment #4)
> > > (In reply to Jonathan Wakely from comment #3)
> > > > Ah, maybe the problem is that the library code manually elides destroying
> > > > the elements, precisely because it's a no-op. So we don't actually destroy
> > > > the elements, which means the compiler might think they're still initialized
> > > > and so could be inspected.
> > > > 
> > > > If the library explicitly does vec[i].~T() for every i then would that help?
> > > > The compiler would know there are no valid elements in the storage, and so
> > > > nothing operator delete could inspect.
> > > > 
> > > > We could continue to elide destroying the elements when !defined
> > > > __OPTIMIZE__ so that we don't run a loop that does nothing, but with
> > > > optimization enabled rely on the compiler to remove that loop.
> > > 
> > > I don't think that would help.  The issue is the compiler thinks that
> > > 
> > > operator delete (_37, _49);
> > > 
> > > uses the memory at _37 and thus the stores
> > > 
> > > *_37 = _24;
> > > 
> > > and
> > > 
> > > __builtin_memmove (_37, pretmp_63, _23);
> > > 
> > > are not dead.
> > 
> > But if the library did _37->~_Tp() to destroy the element at _37 then it
> > would be dead, and accessing the element outside its lifetime would be
> > undefined. The bytes can only be accessed as char, unsigned char or
> > std::byte after that.
> > 
> > > IIRC 'operator delete' (_ZdlPvm in this case), can be
> > > overridden by the user and can inspect the memory state before "releasing"
> > > the storage?
> > 
> > That would be insane for operator delete to do that. Maybe possible, but
> > insane.
> 
> Well, the standard doesn't prohibit "insanity" so we have to generate
> correct code even in those cases, no? 

Indeed.

> After all this is what PR101480
> was about ("insane" stuff) done even to new/delete _expressions_.
> 
> Would it be correct (by the standards wording) to extend the current
> handling of new/delete expressions - which is that 'new' returns a pointer
> not aliasing to any other pointer and that 'delete' does not read from
> the memory pointed to by the first argument and this pointer also does not
> escape to direct calls of operator new/delete?

No, I don't think so. The [expr.new] p13 permission to elide calls to operator
new seems to only apply to new expressions and not direct calls to operator
new.


> Basically in gimple.cc:gimple_call_fnspec we currenty do
> 
>   /* If the call is to a replaceable operator delete and results
>      from a delete expression as opposed to a direct call to
>      such operator, then we can treat it as free.  */
>   if (fndecl
>       && DECL_IS_OPERATOR_DELETE_P (fndecl)
>       && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
>       && gimple_call_from_new_or_delete (stmt))
>     return ". o ";
>   /* Similarly operator new can be treated as malloc.  */
>   if (fndecl
>       && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
>       && gimple_call_from_new_or_delete (stmt))
>     return "m ";
> 
> where the comments do not exactly match the behavior (we also allow
> arbitrary side-effects on global memory).  That we model 'delete'
> as possibly writing to the released storage is so it serves as barrier for
> earlier loads/stores to avoid sinking them across.
> 
> > In any case, the stores to _37 would be dead now, right? So even if operator
> > delete inspects the memory as raw bytes using memcmp or similar, it's
> > reading uninitialized storage, so any value is acceptable. So the stores to
> > _37 should be DSE-able now.
> 
> There's noting in the IL indicating that though (but I haven't updated
> libstd++ a while to create a updated preprocessed source - will do so now).

I'm talking about a potential change to libstdc++ to add _37->~_Tp() so you
won't see it in the IL now.

What I meant in comment 3 is that libstdc++ uses template metaprogramming to
manually elide destroying the elements, because trivial types like 'int' have
no destructor. See libstdc++-v3/include/bits/stl_construct.h lines 155-197:

  template<bool>
    struct _Destroy_aux
    {
      template<typename _ForwardIterator>
        static _GLIBCXX20_CONSTEXPR void
        __destroy(_ForwardIterator __first, _ForwardIterator __last)
        {
          for (; __first != __last; ++__first)
            std::_Destroy(std::__addressof(*__first));
        }
    };

  template<>
    struct _Destroy_aux<true>
    {
      template<typename _ForwardIterator>
        static void
        __destroy(_ForwardIterator, _ForwardIterator) { }
    };

For vector<int> we use the _Destroy_aux<true> specialization, which is a no-op.
So the compiler thinks that the elements of the std::vector<int> are still
alive when we free the storage.

I'm suggesting that if we actually ended the lifetime of the int elements, then
the compiler would see that any stores to _37 are now dead, because we clobber
them before calling operator delete.

e.g. something like:

--- a/libstdc++-v3/include/bits/stl_construct.h
+++ b/libstdc++-v3/include/bits/stl_construct.h
@@ -181,6 +181,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _GLIBCXX20_CONSTEXPR inline void
     _Destroy(_ForwardIterator __first, _ForwardIterator __last)
     {
+#ifdef __OPTIMIZE__
+      // Rely on the compiler to optimize the loop away for trivial types.
+      return _Destroy_aux<false>::__destroy(__first, __last);
+#else
       typedef typename iterator_traits<_ForwardIterator>::value_type
                        _Value_type;
 #if __cplusplus >= 201103L
@@ -194,6 +198,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #endif
       std::_Destroy_aux<__has_trivial_destructor(_Value_type)>::
        __destroy(__first, __last);
+#endif
     }

   template<bool>

But this doesn't change the generated code ¯\_(ツ)_/¯


> OK, then how do we arrange things so the compiler knows it can completely
> elide the unused std::vector?  Or do you say it's not valid to do that?

I think it's valid in theory. I don't know if it's possible for GCC to do it in
practice. There doesn't seem to be anything the library can do to help, so
WONTFIX.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (6 preceding siblings ...)
  2023-04-12 10:01 ` redi at gcc dot gnu.org
@ 2023-04-12 10:13 ` redi at gcc dot gnu.org
  2023-04-12 10:22 ` rguenth at gcc dot gnu.org
                   ` (12 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-12 10:13 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #7)
> I think it's valid in theory. I don't know if it's possible for GCC to do it
> in practice. There doesn't seem to be anything the library can do to help,
> so WONTFIX.

I think it's valid in theory because the implementation of std::vector is
opaque. It's not required to actually call operator new and operator delete to
obtain its storage, and so if the library and compiler collaborate to provide
storage some other way (e.g. using a stack buffer) then that's allowed.

But in practice it's hard to do that.

Maybe it could be done with a new __builtin_elidable_operator_new and
__builtin_elidable_operator_delete pair that the library could use, or
attributes like you suggested in comment 4. That would allow the library to say
"I'm calling operator new and operator delete because I need to obtain memory,
but **these particular invocations** are not required to be visible to the
user, and therefore can be elided in the same way as new expressions and delete
expressions".

In fact Clang's __builtin_operator_new already has exactly those semantics:

https://clang.llvm.org/docs/LanguageExtensions.html#builtin-operator-new-and-builtin-operator-delete

And we already use those builtins if available, see PR 94295.

So maybe what we need is to implement those.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (7 preceding siblings ...)
  2023-04-12 10:13 ` redi at gcc dot gnu.org
@ 2023-04-12 10:22 ` rguenth at gcc dot gnu.org
  2023-04-12 10:29 ` redi at gcc dot gnu.org
                   ` (11 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-04-12 10:22 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #8)
> (In reply to Jonathan Wakely from comment #7)
> > I think it's valid in theory. I don't know if it's possible for GCC to do it
> > in practice. There doesn't seem to be anything the library can do to help,
> > so WONTFIX.
> 
> I think it's valid in theory because the implementation of std::vector is
> opaque. It's not required to actually call operator new and operator delete
> to obtain its storage, and so if the library and compiler collaborate to
> provide storage some other way (e.g. using a stack buffer) then that's
> allowed.
> 
> But in practice it's hard to do that.
> 
> Maybe it could be done with a new __builtin_elidable_operator_new and
> __builtin_elidable_operator_delete pair that the library could use, or
> attributes like you suggested in comment 4. That would allow the library to
> say "I'm calling operator new and operator delete because I need to obtain
> memory, but **these particular invocations** are not required to be visible
> to the user, and therefore can be elided in the same way as new expressions
> and delete expressions".
> 
> In fact Clang's __builtin_operator_new already has exactly those semantics:
> 
> https://clang.llvm.org/docs/LanguageExtensions.html#builtin-operator-new-and-
> builtin-operator-delete
> 
> And we already use those builtins if available, see PR 94295.
> 
> So maybe what we need is to implement those.

Yep, looking at that link this is exactly what would be needed.  Note
that in the middle-end we already see the calls to
DECL_IS_REPLACEABLE_OPERATOR_NEW_P and replaceable operator delete.  We
just restrict all optimizations to calls emitted from new/delete expressions.
I'm not sure the clang builtin is exactly providing the new/delete
expression semantics resolving to replaceable operator new/delete or if
there's other details.  That is, I'm curious whether __builtin_operator_new
provides more guarantees than a new/delete expression - the builtin semantics
isn't very well defined unfortunately.  It might be safer to go our own
way here.

I suppose the standard library could as well use malloc/free?  Or is the
standard library required to perform allocation with "replaceable operator
new/delete"?  (I suppose it helps not open-coding the exceptional cases)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (8 preceding siblings ...)
  2023-04-12 10:22 ` rguenth at gcc dot gnu.org
@ 2023-04-12 10:29 ` redi at gcc dot gnu.org
  2023-04-12 10:35 ` redi at gcc dot gnu.org
                   ` (10 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-12 10:29 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> Yep, looking at that link this is exactly what would be needed.  Note
> that in the middle-end we already see the calls to
> DECL_IS_REPLACEABLE_OPERATOR_NEW_P and replaceable operator delete.  We
> just restrict all optimizations to calls emitted from new/delete expressions.
> I'm not sure the clang builtin is exactly providing the new/delete
> expression semantics resolving to replaceable operator new/delete or if
> there's other details.  That is, I'm curious whether __builtin_operator_new
> provides more guarantees than a new/delete expression - the builtin semantics
> isn't very well defined unfortunately.  It might be safer to go our own
> way here.

I would prefer if we don't deviate, and get Clang to clarify things instead of
reinventing something that looks similar but isn't.

> I suppose the standard library could as well use malloc/free?

No. std::allocator is required to use operator new and operator delete to
obtain memory, but it's unspecified when or how often it calls them. So it's OK
to call them once at startup to obtain ALL THE MEMORY that any std::allocator
will ever need for the duration of the program, or to merge allocations, or to
elide them completely. But it's not OK to use malloc.

std::allocator::allocate says:
"The storage for the array is obtained by calling ::operator new (17.6.3), but
it is unspecified when or how often this function is called."

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (9 preceding siblings ...)
  2023-04-12 10:29 ` redi at gcc dot gnu.org
@ 2023-04-12 10:35 ` redi at gcc dot gnu.org
  2023-04-12 11:52 ` rguenther at suse dot de
                   ` (9 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-12 10:35 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #10)
> I would prefer if we don't deviate, and get Clang to clarify things instead
> of reinventing something that looks similar but isn't.

I suppose as long as the common subset of semantics that the library cares
about works the same way, it's OK.

It would be bad if the library needs to use extra checks beyond __has_builtin
to discover which "flavour" of __builtin_operator_new is present.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (10 preceding siblings ...)
  2023-04-12 10:35 ` redi at gcc dot gnu.org
@ 2023-04-12 11:52 ` rguenther at suse dot de
  2023-04-12 11:55 ` redi at gcc dot gnu.org
                   ` (8 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: rguenther at suse dot de @ 2023-04-12 11:52 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #12 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 12 Apr 2023, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
> 
> --- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> (In reply to Jonathan Wakely from comment #10)
> > I would prefer if we don't deviate, and get Clang to clarify things instead
> > of reinventing something that looks similar but isn't.
> 
> I suppose as long as the common subset of semantics that the library cares
> about works the same way, it's OK.
> 
> It would be bad if the library needs to use extra checks beyond __has_builtin
> to discover which "flavour" of __builtin_operator_new is present.

So if we can get clarification that __builtin_operator_new/delete
allow at least all transforms as if using a new or delete expression
then we can use the same representation as that in the middle-end
for now (just set the CALL_FROM_NEW_OR_DELETE_P flag).

If the builtins provide _additional_ guarantees then we can make use
of those only when we extend things on the middle-end side.

Now we then only need to make the C++ frontend "mangle" the
__builtin_operator_new/delete calls to the "correct" allocation function.
I suppose the goal is to resolve to exactly the same symbols as
when not using the builtins.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (11 preceding siblings ...)
  2023-04-12 11:52 ` rguenther at suse dot de
@ 2023-04-12 11:55 ` redi at gcc dot gnu.org
  2023-04-16 18:38 ` richard-gccbugzilla at metafoo dot co.uk
                   ` (7 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2023-04-12 11:55 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |richard-gccbugzilla@metafoo
                   |                            |.co.uk

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
@zygoloid, are you the right person to ask about __builtin_operator_new
semantics? See the previous comment (comment 12)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (12 preceding siblings ...)
  2023-04-12 11:55 ` redi at gcc dot gnu.org
@ 2023-04-16 18:38 ` richard-gccbugzilla at metafoo dot co.uk
  2023-04-17  6:49 ` rguenth at gcc dot gnu.org
                   ` (6 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: richard-gccbugzilla at metafoo dot co.uk @ 2023-04-16 18:38 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #14 from Richard Smith <richard-gccbugzilla at metafoo dot co.uk> ---
If I understand correctly, you're looking for documentation that

  __builtin_operator_new(size)

has the exact same semantics and permits the same optimizations as `::new T`
for a trivially-constructible non-array type `T` whose size is `size` and that

  __builtin_operator_delete(p)

has the exact same semantics and permits the same optimizations as `::delete p`
for a trivially-destructible non-array type `T` whose size is `size`, with `p`
of type `T*` -- and similarly for the other (aligned, nothrow) variants?

That is the intent; I can look into getting Clang's documentation updated to
say that more explicitly if that's useful to you.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (13 preceding siblings ...)
  2023-04-16 18:38 ` richard-gccbugzilla at metafoo dot co.uk
@ 2023-04-17  6:49 ` rguenth at gcc dot gnu.org
  2023-04-18  2:15 ` richard-gccbugzilla at metafoo dot co.uk
                   ` (5 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-04-17  6:49 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Smith from comment #14)
> If I understand correctly, you're looking for documentation that
> 
>   __builtin_operator_new(size)
> 
> has the exact same semantics and permits the same optimizations as `::new T`
> for a trivially-constructible non-array type `T` whose size is `size` and
> that
> 
>   __builtin_operator_delete(p)
> 
> has the exact same semantics and permits the same optimizations as `::delete
> p` for a trivially-destructible non-array type `T` whose size is `size`,
> with `p` of type `T*` -- and similarly for the other (aligned, nothrow)
> variants?
> 
> That is the intent; I can look into getting Clang's documentation updated to
> say that more explicitly if that's useful to you.

I was specifically looking at C++20 7.6.2.7/10 to /14 (but maybe also
others and of course the relevant parts of the delete expression).  In
particular the extra leeway the standard provides to new/delete _expressions_
vs. calls of the global replaceable operators directly - do the
__builtin_operator_{new,delete} in this regard behave like new/delete
_expressions_ or like direct calls to the operators?

Do the builtins call one of the replaceable global new/delete operators
and thus users can reliably override them?

How do the builtins behave during constexpr evaluation?  new/delete
expressions have their behavior documented in the standard.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (14 preceding siblings ...)
  2023-04-17  6:49 ` rguenth at gcc dot gnu.org
@ 2023-04-18  2:15 ` richard-gccbugzilla at metafoo dot co.uk
  2023-06-15 18:38 ` hiraditya at msn dot com
                   ` (4 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: richard-gccbugzilla at metafoo dot co.uk @ 2023-04-18  2:15 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #16 from Richard Smith <richard-gccbugzilla at metafoo dot co.uk> ---
(In reply to Richard Biener from comment #15)
> I was specifically looking at C++20 7.6.2.7/10 to /14 (but maybe also
> others and of course the relevant parts of the delete expression).  In
> particular the extra leeway the standard provides to new/delete _expressions_
> vs. calls of the global replaceable operators directly - do the
> __builtin_operator_{new,delete} in this regard behave like new/delete
> _expressions_ or like direct calls to the operators?

They permit the same optimizations as new/delete expressions. The sections of
the C++ standard that you referred to are what the documentation for the
builtins means when it says:

"[__builtin_operator_new] allows certain optimizations that the C++ standard
does not permit for a direct function call to ::operator new (in particular,
removing new / delete pairs and merging allocations)"

> Do the builtins call one of the replaceable global new/delete operators
> and thus users can reliably override them?

If the implementation doesn't provide storage in some other way, optimize away
the allocation, or merge it with another allocation, then yes, the storage is
obtained by calling the corresponding replaceable global new/delete operators.
Per the documentation:

"A call to __builtin_operator_new(args) is exactly the same as a call to
::operator new(args), except that [text quoted above]"

> How do the builtins behave during constexpr evaluation?  new/delete
> expressions have their behavior documented in the standard.

They behave exactly like a direct call to the replaceable global allocation
function.

In Clang's implementation, direct calls to ::operator new and ::operator delete
are permitted within calls to std::allocator<T>::allocate and
std::allocator<T>::deallocate during constant evaluation, and are treated as
allocating or deallocating arrays of T; consequently, calls to
__builtin_operator_new and __builtin_operator_delete are permitted in the same
contexts. In all other contexts, Clang rejects such calls, because the callee
is not declared `constexpr`.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (15 preceding siblings ...)
  2023-04-18  2:15 ` richard-gccbugzilla at metafoo dot co.uk
@ 2023-06-15 18:38 ` hiraditya at msn dot com
  2024-05-11  0:07 ` xry111 at gcc dot gnu.org
                   ` (3 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: hiraditya at msn dot com @ 2023-06-15 18:38 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #17 from AK <hiraditya at msn dot com> ---
With recent changes in libc++ (https://reviews.llvm.org/D147741) clang
optimizes away the new-delete pair. https://godbolt.org/z/a6PG54Pvb

$ clang++ -O3 -stdlib=libc++ -fno-exceptions

vat1(std::__1::vector<int, std::__1::allocator<int> >): #
@vat1(std::__1::vector<int, std::__1::allocator<int> >)
  sub rsp, 24
  xorps xmm0, xmm0
  movaps xmmword ptr [rsp], xmm0
  mov qword ptr [rsp + 16], 0
  mov rax, qword ptr [rdi + 8]
  sub rax, qword ptr [rdi]
  je .LBB0_2
  js .LBB0_3
.LBB0_2:
  mov eax, 10
  add rsp, 24
  ret
.LBB0_3:
  mov rdi, rsp
  call std::__1::vector<int, std::__1::allocator<int>
>::__throw_length_error[abi:v170000]() const
.L.str:
  .asciz "vector"

.L.str.1:
  .asciz "length_error was thrown in -fno-exceptions mode with message \"%s\""


Previously clang couldn't even convert the copy to a memmove and would generate
a raw loop e.g., https://godbolt.org/z/G8ax1o5bc

.LBB0_6: # =>This Inner Loop Header: Depth=1
  movups xmm0, xmmword ptr [r15 + 4*rdi]
  movups xmm1, xmmword ptr [r15 + 4*rdi + 16]
  movups xmmword ptr [rax + 4*rdi], xmm0
  movups xmmword ptr [rax + 4*rdi + 16], xmm1
  add rdi, 8
  cmp rsi, rdi
  jne .LBB0_6
  cmp rbx, rsi
  jne .LBB0_8
  jmp .LBB0_9
.LBB0_3:

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (16 preceding siblings ...)
  2023-06-15 18:38 ` hiraditya at msn dot com
@ 2024-05-11  0:07 ` xry111 at gcc dot gnu.org
  2024-05-11 16:05 ` hubicka at gcc dot gnu.org
                   ` (2 subsequent siblings)
  20 siblings, 0 replies; 23+ messages in thread
From: xry111 at gcc dot gnu.org @ 2024-05-11  0:07 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org

--- Comment #18 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
*** Bug 115037 has been marked as a duplicate of this bug. ***

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (17 preceding siblings ...)
  2024-05-11  0:07 ` xry111 at gcc dot gnu.org
@ 2024-05-11 16:05 ` hubicka at gcc dot gnu.org
  2024-05-14 13:47   ` Jan Hubicka
  2024-05-11 16:44 ` redi at gcc dot gnu.org
  2024-05-14 13:47 ` hubicka at ucw dot cz
  20 siblings, 1 reply; 23+ messages in thread
From: hubicka at gcc dot gnu.org @ 2024-05-11 16:05 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #19 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Note that the testcase from PR115037 also shows that we are not able to
optimize out dead stores to the vector, which is another quite noticeable
problem.

void
test()
{
        std::vector<int> test;
        test.push_back (1);
}

We alocate the block, store 1 and immediately delete it.
void test ()
{
  int * test$D25839$_M_impl$D25146$_M_start;
  struct vector test;
  int * _61;

  <bb 2> [local count: 1073741824]:
  _61 = operator new (4);

  <bb 3> [local count: 1063439392]:
  *_61 = 1;
  operator delete (_61, 4);
  test ={v} {CLOBBER};
  test ={v} {CLOBBER(eol)};
  return;

  <bb 4> [count: 0]:
<L1>:
  test ={v} {CLOBBER};
  resx 2

}

So my understanding is that we decided to not optimize away the dead stores
since the particular operator delete does not pass test:

  /* If the call is to a replaceable operator delete and results
     from a delete expression as opposed to a direct call to
     such operator, then we can treat it as free.  */
  if (fndecl
      && DECL_IS_OPERATOR_DELETE_P (fndecl)
      && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
      && gimple_call_from_new_or_delete (stmt))
    return ". o ";

This is because we believe that operator delete may be implemented in an insane
way that inspects the values stored in the block being freed.

I can sort of see that one can write standard conforming code that allocates
some data that is POD and inspects it in destructor.
However for std::vector this argument is not really applicable. Standard does
specify that new/delete is used to allocate/deallocate the memory but does not
say how the memory is organized or what happens before deallocation.
(i.e. it is probably valid for std::vector to memset the block just before
deallocating it).

Similar argument can IMO be used for eliding unused memory allocations. It is
kind of up to std::vector implementation on how many allocations/deallocations
it does, right?

So we need a way to annotate the new/delete calls in the standard library as
safe for such optimizations (i.e. implement clang's
__bulitin_operator_new/delete?)

How clang manages to optimize this out without additional hinting?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (18 preceding siblings ...)
  2024-05-11 16:05 ` hubicka at gcc dot gnu.org
@ 2024-05-11 16:44 ` redi at gcc dot gnu.org
  2024-05-14 13:47 ` hubicka at ucw dot cz
  20 siblings, 0 replies; 23+ messages in thread
From: redi at gcc dot gnu.org @ 2024-05-11 16:44 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #20 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #19)
> Similar argument can IMO be used for eliding unused memory allocations. It
> is kind of up to std::vector implementation on how many
> allocations/deallocations it does, right?

It's up to std::allocator, which is not required to call operator new every
time memory is needed. 


> So we need a way to annotate the new/delete calls in the standard library as
> safe for such optimizations (i.e. implement clang's
> __bulitin_operator_new/delete?)

Yes, see PR 110137.

> How clang manages to optimize this out without additional hinting?

It supports __builtin_operator_{new,delete} and libstdc++ uses that when
compiled with clang.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2024-05-11 16:05 ` hubicka at gcc dot gnu.org
@ 2024-05-14 13:47   ` Jan Hubicka
  0 siblings, 0 replies; 23+ messages in thread
From: Jan Hubicka @ 2024-05-14 13:47 UTC (permalink / raw)
  To: hubicka at gcc dot gnu.org; +Cc: gcc-bugs

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

This patch attempts to add __builtin_operator_new/delete. So far they
are not optimized, which will need to be done by extra flag of BUILT_IN_
code.  also the decl.cc code can be refactored to be less of cut&paste
and I guess has_builtin hack to return proper value needs to be moved
to C++ FE.

However the immediate problem I run into is that libstdc++ testuiste
fails due to lack of std::nothrow overrides.  I wonder how to get that
working?

[-- Attachment #2: newdelete --]
[-- Type: text/plain, Size: 5119 bytes --]

diff --git a/gcc/c-family/c-lex.cc b/gcc/c-family/c-lex.cc
index ff5ce2bf729..602b097059c 100644
--- a/gcc/c-family/c-lex.cc
+++ b/gcc/c-family/c-lex.cc
@@ -533,6 +533,10 @@ c_common_has_builtin (cpp_reader *pfile)
   if (!name)
     return 0;
 
+  if (!strcmp (name, "__builtin_operator_new")
+      || !strcmp (name, "__builtin_operator_delete"))
+    return 201802L;
+
   return names_builtin_p (name);
 }
 
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 3fc8835154d..90b100ca3dc 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -59,6 +59,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "omp-general.h"
 #include "omp-offload.h"  /* For offload_vars.  */
 #include "opts.h"
+#include "print-tree.h"
 #include "langhooks-def.h"  /* For lhd_simulate_record_decl  */
 
 /* Possible cases of bad specifiers type used by bad_specifiers. */
@@ -5048,13 +5049,27 @@ cxx_init_decl_processing (void)
     DECL_IS_MALLOC (opnew) = 1;
     DECL_SET_IS_OPERATOR_NEW (opnew, true);
     DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
+    tree builtin_opnew = build_cp_library_fn (get_identifier("__builtin_operator_new"),
+					      NEW_EXPR, newtype, 0);
+    DECL_IS_MALLOC (builtin_opnew) = 1;
+    DECL_SET_IS_OPERATOR_NEW (builtin_opnew, true);
+    DECL_IS_REPLACEABLE_OPERATOR (builtin_opnew) = 1;
+    SET_DECL_ASSEMBLER_NAME (builtin_opnew, DECL_ASSEMBLER_NAME (opnew));
+    pushdecl (builtin_opnew);
     opnew = push_cp_library_fn (VEC_NEW_EXPR, newtype, 0);
     DECL_IS_MALLOC (opnew) = 1;
     DECL_SET_IS_OPERATOR_NEW (opnew, true);
     DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
+
     tree opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
     DECL_SET_IS_OPERATOR_DELETE (opdel, true);
     DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+    tree builtin_opdel = build_cp_library_fn (get_identifier("__builtin_operator_delete"),
+					      DELETE_EXPR, deltype, ECF_NOTHROW);
+    DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+    DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+    SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+    pushdecl (builtin_opdel);
     opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
     DECL_SET_IS_OPERATOR_DELETE (opdel, true);
     DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
@@ -5072,6 +5087,12 @@ cxx_init_decl_processing (void)
 	opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
 	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+	builtin_opdel = build_cp_library_fn (get_identifier("__builtin_operator_delete"),
+					     DELETE_EXPR, deltype, ECF_NOTHROW);
+	DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+	SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+	pushdecl (builtin_opdel);
 	opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
 	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
@@ -5094,6 +5115,13 @@ cxx_init_decl_processing (void)
 	DECL_IS_MALLOC (opnew) = 1;
 	DECL_SET_IS_OPERATOR_NEW (opnew, true);
 	DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
+	builtin_opnew = build_cp_library_fn (get_identifier("__builtin_operator_new"),
+					     VEC_NEW_EXPR, newtype, 0);
+	DECL_IS_MALLOC (builtin_opnew) = 1;
+	DECL_SET_IS_OPERATOR_NEW (builtin_opnew, true);
+	DECL_IS_REPLACEABLE_OPERATOR (builtin_opnew) = 1;
+	SET_DECL_ASSEMBLER_NAME (builtin_opnew, DECL_ASSEMBLER_NAME (opnew));
+	pushdecl (builtin_opnew);
 	opnew = push_cp_library_fn (VEC_NEW_EXPR, newtype, 0);
 	DECL_IS_MALLOC (opnew) = 1;
 	DECL_SET_IS_OPERATOR_NEW (opnew, true);
@@ -5107,6 +5135,12 @@ cxx_init_decl_processing (void)
 	opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
 	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+	builtin_opdel = build_cp_library_fn (get_identifier("__builtin_operator_delete"),
+					     DELETE_EXPR, deltype, ECF_NOTHROW);
+	DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+	SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+	pushdecl (builtin_opdel);
 	opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
 	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
@@ -5122,6 +5156,12 @@ cxx_init_decl_processing (void)
 	    opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	    DECL_SET_IS_OPERATOR_DELETE (opdel, true);
 	    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+	    builtin_opdel = build_cp_library_fn (get_identifier("__builtin_operator_delete"),
+						 DELETE_EXPR, deltype, ECF_NOTHROW);
+	    DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+	    DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+	    SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+	    pushdecl (builtin_opdel);
 	    opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	    DECL_SET_IS_OPERATOR_DELETE (opdel, true);
 	    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
  2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
                   ` (19 preceding siblings ...)
  2024-05-11 16:44 ` redi at gcc dot gnu.org
@ 2024-05-14 13:47 ` hubicka at ucw dot cz
  20 siblings, 0 replies; 23+ messages in thread
From: hubicka at ucw dot cz @ 2024-05-14 13:47 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442

--- Comment #21 from Jan Hubicka <hubicka at ucw dot cz> ---
This patch attempts to add __builtin_operator_new/delete. So far they
are not optimized, which will need to be done by extra flag of BUILT_IN_
code.  also the decl.cc code can be refactored to be less of cut&paste
and I guess has_builtin hack to return proper value needs to be moved
to C++ FE.

However the immediate problem I run into is that libstdc++ testuiste
fails due to lack of std::nothrow overrides.  I wonder how to get that
working?

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2024-05-14 13:47 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-06 19:00 [Bug tree-optimization/109442] New: Dead local copy of std::vector not removed from function hiraditya at msn dot com
2023-04-11 13:28 ` [Bug tree-optimization/109442] " rguenth at gcc dot gnu.org
2023-04-11 13:33 ` redi at gcc dot gnu.org
2023-04-11 13:38 ` redi at gcc dot gnu.org
2023-04-12  7:40 ` [Bug libstdc++/109442] " rguenth at gcc dot gnu.org
2023-04-12  9:29 ` redi at gcc dot gnu.org
2023-04-12  9:45 ` rguenth at gcc dot gnu.org
2023-04-12 10:01 ` redi at gcc dot gnu.org
2023-04-12 10:13 ` redi at gcc dot gnu.org
2023-04-12 10:22 ` rguenth at gcc dot gnu.org
2023-04-12 10:29 ` redi at gcc dot gnu.org
2023-04-12 10:35 ` redi at gcc dot gnu.org
2023-04-12 11:52 ` rguenther at suse dot de
2023-04-12 11:55 ` redi at gcc dot gnu.org
2023-04-16 18:38 ` richard-gccbugzilla at metafoo dot co.uk
2023-04-17  6:49 ` rguenth at gcc dot gnu.org
2023-04-18  2:15 ` richard-gccbugzilla at metafoo dot co.uk
2023-06-15 18:38 ` hiraditya at msn dot com
2024-05-11  0:07 ` xry111 at gcc dot gnu.org
2024-05-11 16:05 ` hubicka at gcc dot gnu.org
2024-05-14 13:47   ` Jan Hubicka
2024-05-11 16:44 ` redi at gcc dot gnu.org
2024-05-14 13:47 ` hubicka at ucw dot cz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).