[Bug c++/97151] New: GCC fails to optimize away uselessly allocated arrays (C++)

[Bug c++/97151] New: GCC fails to optimize away uselessly allocated arrays (C++)

[marat at slonopotamus dot org]

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

            Bug ID: 97151
           Summary: GCC fails to optimize away uselessly allocated arrays
                    (C++)
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marat at slonopotamus dot org
  Target Milestone: ---

A test program:

int main()
{
    int *a = new int[10]();
    delete[] a;

    return 0;
}


Tested against: gcc 10.2.0 and gcc trunk as of 20200920 on codebolt.org with
-O2, -O3 and -Ofast

Expected: new/delete are optimized away under -O2 and higher because "a" is
never used for anything useful (like, side effects or whatever).

Actual: new/delete are present in assembly:

main:
        sub     rsp, 8
        mov     edi, 40
        call    operator new[](unsigned long)
        pxor    xmm0, xmm0
        mov     QWORD PTR [rax+32], 0
        mov     rdi, rax
        movups  XMMWORD PTR [rax], xmm0
        movups  XMMWORD PTR [rax+16], xmm0
        call    operator delete[](void*)
        xor     eax, eax
        add     rsp, 8
        ret

Interesting observations:

1. if "delete[]" is commented out, whole program is optimized away
2. if "new int[10]()" is replaced with "new int[10]", whole program is
optimized away
3. if new/delete is replaced with matching malloc/free, whole program is
optimized away

If someone wants to play with this example online, here it is:
https://godbolt.org/z/YoGzxo

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[marat at slonopotamus dot org]

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

--- Comment #1 from Marat Radchenko <marat at slonopotamus dot org> ---
Another program that exhibits the same behavior:

int main()
{
    int *a = new int[10];
    a[0] = 0;
    delete[] a;

    return 0;
}

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marxin at gcc dot gnu.org
           Keywords|                            |missed-optimization

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
We've iterated a lot on new/delete pair removal, I guess Martin can quickly
figure out why this particular case isn't handled.

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[marxin at gcc dot gnu.org]

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

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
It's caused by what was mentioned as 2) which transforms to:

  _6 = operator new [] (40);
  __builtin_memset (_6, 0, 40);
  operator delete [] (_6);

So there's a use of _6 and we probably can't remove it as memset can
potentially access NULL pointer if operator new [] fails.

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[marat at slonopotamus dot org]

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

--- Comment #4 from Marat Radchenko <marat at slonopotamus dot org> ---
(In reply to Martin Liška from comment #3)
> It's caused by what was mentioned as 2) which transforms to:
> 
>   _6 = operator new [] (40);
>   __builtin_memset (_6, 0, 40);
>   operator delete [] (_6);
> 
> So there's a use of _6 and we probably can't remove it as memset can
> potentially access NULL pointer if operator new [] fails.

1. If operator new[] fails, there would be an exception instead of NULL, no?

2. It doesn't matter, you may guard accessing array with NULL check and still
observe described behavior.

What puzzles me the most is why removal of delete[] allows it to optimize.

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, operator new[](40) cannot return null, and passing null to memset is
undefined, so we could assume it is never called with null.

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Marat Radchenko from comment #4)
> (In reply to Martin Liška from comment #3)
> > It's caused by what was mentioned as 2) which transforms to:
> > 
> >   _6 = operator new [] (40);
> >   __builtin_memset (_6, 0, 40);
> >   operator delete [] (_6);
> > 
> > So there's a use of _6 and we probably can't remove it as memset can
> > potentially access NULL pointer if operator new [] fails.
> 
> 1. If operator new[] fails, there would be an exception instead of NULL, no?
> 
> 2. It doesn't matter, you may guard accessing array with NULL check and
> still observe described behavior.
> 
> What puzzles me the most is why removal of delete[] allows it to optimize.

Very likely delete[] is keeping the memset itself live.  In particular
early CDDCE can elide the loop without the delete[] but with the delete[]
we see

Marking useful stmt: operator delete [] (_7);

so that's a no-go there.  Using malloc/free we manage to delete the loop early
because its argument is not considered necessary.  It looks like points-to
analysis fails to fully handle new/delete and causes the allocated storage
to point to escaped memory:

_7 = &HEAP(10)

new[] handled OK

ESCAPED = _7

delete[] not handled.  One "easy" way would be for the C++ FE to annotate
delete[] with fnspec ".w" to mark the argument as not escaping.  A hackish
patch in PTA also fixes it:

diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 44fe52e0f65..f676bf91e95 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -4857,6 +4857,8 @@ find_func_aliases_for_call (struct function *fn, gcall
*t)
         point for reachable memory of their arguments.  */
       else if (flags & (ECF_PURE|ECF_LOOPING_CONST_OR_PURE))
        handle_pure_call (t, &rhsc);
+      else if (fndecl && DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (fndecl))
+       ;
       else
        handle_rhs_call (t, &rhsc);
       if (gimple_call_lhs (t))

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #6)
> (In reply to Marat Radchenko from comment #4)
> > (In reply to Martin Liška from comment #3)
> > > It's caused by what was mentioned as 2) which transforms to:
> > > 
> > >   _6 = operator new [] (40);
> > >   __builtin_memset (_6, 0, 40);
> > >   operator delete [] (_6);
> > > 
> > > So there's a use of _6 and we probably can't remove it as memset can
> > > potentially access NULL pointer if operator new [] fails.
> > 
> > 1. If operator new[] fails, there would be an exception instead of NULL, no?
> > 
> > 2. It doesn't matter, you may guard accessing array with NULL check and
> > still observe described behavior.
> > 
> > What puzzles me the most is why removal of delete[] allows it to optimize.
> 
> Very likely delete[] is keeping the memset itself live.  In particular
> early CDDCE can elide the loop without the delete[] but with the delete[]
> we see
> 
> Marking useful stmt: operator delete [] (_7);
> 
> so that's a no-go there.  Using malloc/free we manage to delete the loop
> early
> because its argument is not considered necessary.  It looks like points-to
> analysis fails to fully handle new/delete and causes the allocated storage
> to point to escaped memory:
> 
> _7 = &HEAP(10)
> 
> new[] handled OK
> 
> ESCAPED = _7
> 
> delete[] not handled.  One "easy" way would be for the C++ FE to annotate
> delete[] with fnspec ".w" to mark the argument as not escaping.  A hackish
> patch in PTA also fixes it:
> 
> diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
> index 44fe52e0f65..f676bf91e95 100644
> --- a/gcc/tree-ssa-structalias.c
> +++ b/gcc/tree-ssa-structalias.c
> @@ -4857,6 +4857,8 @@ find_func_aliases_for_call (struct function *fn, gcall
> *t)
>          point for reachable memory of their arguments.  */
>        else if (flags & (ECF_PURE|ECF_LOOPING_CONST_OR_PURE))
>         handle_pure_call (t, &rhsc);
> +      else if (fndecl && DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (fndecl))
> +       ;
>        else
>         handle_rhs_call (t, &rhsc);
>        if (gimple_call_lhs (t))

passes bootstrap & regtest, also improving g++.dg/cpp1y/new1.C as of

PASS: g++.dg/cpp1y/new1.C  -std=gnu++98 (test for excess errors)
g++.dg/cpp1y/new1.C  -std=gnu++98 : pattern found 6 times
FAIL: g++.dg/cpp1y/new1.C  -std=gnu++98  scan-tree-dump-times cddce1 "Deleting
: operator delete" 5
g++.dg/cpp1y/new1.C  -std=gnu++98 : pattern found 8 times
FAIL: g++.dg/cpp1y/new1.C  -std=gnu++98  scan-tree-dump-times cddce1 "Deleting
: _\\d+ = operator new" 7

note there are 10 functions in new1.C and there's no XFAIL so it's not clear
whether the currently not optimized cases are to be not optimized for
correctness or whether those are just missed optimizations...

The patch might be even OK in case DECL_IS_REPLACEABLE_OPERATOR_DELETE_P
implies that there is no return value and no argument escapes (IIRC there's
only ever a single argument to operator delete).  Note the patch implies
that the storage is not written to, but I suppose its contents become
undefined after 'delete'.

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #7)
> (IIRC there's
> only ever a single argument to operator delete).

That's not always true. See -fsized-deallocation

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:9e64f17d044767248175fece80a2759d94c45fc4

commit r11-3386-g9e64f17d044767248175fece80a2759d94c45fc4
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Sep 23 10:11:03 2020 +0200

    tree-optimization/97151 - improve PTA for C++ operator delete

    C++ operator delete, when DECL_IS_REPLACEABLE_OPERATOR_DELETE_P,
    does not cause the deleted object to be escaped.  It also has no
    other interesting side-effects for PTA so skip it like we do
    for BUILT_IN_FREE.

    2020-09-23  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/97151
            * tree-ssa-structalias.c (find_func_aliases_for_call):
            DECL_IS_REPLACEABLE_OPERATOR_DELETE_P has no effect on
            arguments.

            * g++.dg/cpp1y/new1.C: Adjust for two more handled transforms.

[Bug c++/97151] GCC fails to optimize away uselessly allocated arrays (C++)

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk.