[PATCH] Check DECL_CONTEXT of new/delete operators.

[PATCH] Check DECL_CONTEXT of new/delete operators.

[Martin Liška]

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

Hi.

The patch ensures that a deleted new/delete pair has a same context.
That will fix the issue presented in the PR.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2020-03-30  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* tree-ssa-dce.c (propagate_necessity): Verify that
	DECL_CONTEXT of a new/delete operators do match.

gcc/testsuite/ChangeLog:

2020-03-30  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* g++.dg/pr94314.C: New test.
---
  gcc/testsuite/g++.dg/pr94314.C | 84 ++++++++++++++++++++++++++++++++++
  gcc/tree-ssa-dce.c             | 31 +++++++++----
  2 files changed, 105 insertions(+), 10 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/pr94314.C



[-- Attachment #2: 0001-Check-DECL_CONTEXT-of-new-delete-operators.patch --]
[-- Type: text/x-patch, Size: 3182 bytes --]

diff --git a/gcc/testsuite/g++.dg/pr94314.C b/gcc/testsuite/g++.dg/pr94314.C
new file mode 100644
index 00000000000..76cd9d8d2a4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314.C
@@ -0,0 +1,84 @@
+/* PR c++/94314.  */
+/* { dg-options "-O2 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+struct A
+{
+  __attribute__((malloc,noinline))
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int A::count = 0;
+
+struct B
+{
+  __attribute__((malloc,noinline))
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  __attribute__((noinline))
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int B::count = 0;
+
+struct C
+{
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int C::count = 0;
+
+int main(){
+  delete new A;
+  if (A::count != 0)
+    __builtin_abort ();
+
+  delete new B;
+  if (B::count != 0)
+    __builtin_abort ();
+
+  delete new C;
+  if (C::count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 1 "cddce1"} } */
+/* { dg-final { scan-tree-dump-times "Deleting : B::operator delete" 1 "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e4077b58890..d86234ead23 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -824,16 +824,27 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      /* Verify that new and delete operators have the same
+			 context.  */
+		      if (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)
+			  && (DECL_CONTEXT (def_callee)
+			      != DECL_CONTEXT (gimple_call_fndecl (stmt))))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Mon, Mar 30, 2020 at 10:41 AM Martin Liška <mliska@suse.cz> wrote:
>
> Hi.
>
> The patch ensures that a deleted new/delete pair has a same context.
> That will fix the issue presented in the PR.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

I think this will break the DCE with LTO since we strip quite some
DECL_CONTEXT in free-lang-data.

Honza - do you remember why we strip DECL_CONTEXT for methods
which will most likely keep their containing record types live through
their this
arguments?  Quoting:

  /* We need to keep field decls associated with their trees. Otherwise tree
     merging may merge some fileds and keep others disjoint wich in turn will
     not do well with TREE_CHAIN pointers linking them.

     Also do not drop containing types for virtual methods and tables because
     these are needed by devirtualization.
     C++ destructors are special because C++ frontends sometimes produces
     virtual destructor as an alias of non-virtual destructor.  In
     devirutalization code we always walk through aliases and we need
     context to be preserved too.  See PR89335  */
  if (TREE_CODE (decl) != FIELD_DECL
      && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
          || (!DECL_VIRTUAL_P (decl)
              && (TREE_CODE (decl) != FUNCTION_DECL
                  || !DECL_CXX_DESTRUCTOR_P (decl)))))
    DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl));

and fld_decl_context stripping up to the next non-TYPE context (unless VLA).

Richard.

> Ready to be installed?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2020-03-30  Martin Liska  <mliska@suse.cz>
>
>         PR c++/94314
>         * tree-ssa-dce.c (propagate_necessity): Verify that
>         DECL_CONTEXT of a new/delete operators do match.
>
> gcc/testsuite/ChangeLog:
>
> 2020-03-30  Martin Liska  <mliska@suse.cz>
>
>         PR c++/94314
>         * g++.dg/pr94314.C: New test.
> ---
>   gcc/testsuite/g++.dg/pr94314.C | 84 ++++++++++++++++++++++++++++++++++
>   gcc/tree-ssa-dce.c             | 31 +++++++++----
>   2 files changed, 105 insertions(+), 10 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/pr94314.C
>
>

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Marc Glisse]

On Mon, 30 Mar 2020, Martin Liška wrote:

> The patch ensures that a deleted new/delete pair has a same context.
> That will fix the issue presented in the PR.

DECL_CONTEXT seems good for that example (assuming it is still available 
in the middle-end), but shouldn't we also check if both are array 
versions? I can build a similar example by implementing the global 
operator new[] in terms of operator new and operator delete[] in terms of 
operator delete. Checking if both are aligned versions could be nice as 
well. I don't know what others can be relevant, I think we are allowed to 
mix throw and nothrow, or sized and non-sized versions.

For DECL_CONTEXT, if we use new on a derived class and delete on a base 
class, I guess it is sensible not to optimize unless devirt / inlining 
manage to show a matched pair of operators, so your patch looks good here.

I understand this feature is getting rather painful, sorry...

-- 
Marc Glisse

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jan Hubicka]

> On Mon, Mar 30, 2020 at 10:41 AM Martin Liška <mliska@suse.cz> wrote:
> >
> > Hi.
> >
> > The patch ensures that a deleted new/delete pair has a same context.
> > That will fix the issue presented in the PR.
> >
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> I think this will break the DCE with LTO since we strip quite some
> DECL_CONTEXT in free-lang-data.
> 
> Honza - do you remember why we strip DECL_CONTEXT for methods
> which will most likely keep their containing record types live through
> their this
> arguments?  Quoting:
> 
>   /* We need to keep field decls associated with their trees. Otherwise tree
>      merging may merge some fileds and keep others disjoint wich in turn will
>      not do well with TREE_CHAIN pointers linking them.
> 
>      Also do not drop containing types for virtual methods and tables because
>      these are needed by devirtualization.
>      C++ destructors are special because C++ frontends sometimes produces
>      virtual destructor as an alias of non-virtual destructor.  In
>      devirutalization code we always walk through aliases and we need
>      context to be preserved too.  See PR89335  */
>   if (TREE_CODE (decl) != FIELD_DECL
>       && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
>           || (!DECL_VIRTUAL_P (decl)
>               && (TREE_CODE (decl) != FUNCTION_DECL
>                   || !DECL_CXX_DESTRUCTOR_P (decl)))))
>     DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl));
> 
> and fld_decl_context stripping up to the next non-TYPE context (unless VLA).

Well, I basically went through all pointers and tried to get rid of as
many of them as possible.  CONTEXT pointers do increase size of SCCs
that increases chance they will not get merged and also processing time
of merging algorithm.  I guess if we need to stream more contexts we
could do that (and check the effect on merging and average SCC size)

Honza
> 
> Richard.
> 
> > Ready to be installed?
> > Thanks,
> > Martin
> >
> > gcc/ChangeLog:
> >
> > 2020-03-30  Martin Liska  <mliska@suse.cz>
> >
> >         PR c++/94314
> >         * tree-ssa-dce.c (propagate_necessity): Verify that
> >         DECL_CONTEXT of a new/delete operators do match.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2020-03-30  Martin Liska  <mliska@suse.cz>
> >
> >         PR c++/94314
> >         * g++.dg/pr94314.C: New test.
> > ---
> >   gcc/testsuite/g++.dg/pr94314.C | 84 ++++++++++++++++++++++++++++++++++
> >   gcc/tree-ssa-dce.c             | 31 +++++++++----
> >   2 files changed, 105 insertions(+), 10 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/pr94314.C
> >
> >

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Martin Liška]

On 3/31/20 2:29 PM, Jan Hubicka wrote:
> Well, I basically went through all pointers and tried to get rid of as
> many of them as possible.  CONTEXT pointers do increase size of SCCs
> that increases chance they will not get merged and also processing time
> of merging algorithm.  I guess if we need to stream more contexts we
> could do that (and check the effect on merging and average SCC size)

Ok, do we want to stream contexts just for the new/delete operators?
Can you please prepare a streaming patch?

Thank you,
Martin

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jan Hubicka]

> On 3/31/20 2:29 PM, Jan Hubicka wrote:
> > Well, I basically went through all pointers and tried to get rid of as
> > many of them as possible.  CONTEXT pointers do increase size of SCCs
> > that increases chance they will not get merged and also processing time
> > of merging algorithm.  I guess if we need to stream more contexts we
> > could do that (and check the effect on merging and average SCC size)
> 
> Ok, do we want to stream contexts just for the new/delete operators?
> Can you please prepare a streaming patch?
Hi,
I am still not convinced that context is useful here.
It took me a while to understand what the code does and why it fails,
but here is a testcase.
It fails for me with your patch and -O2 --param early-inlining-insns=100

The invalid transform is to remove pair
base:new and B:delete
B:new gets inlined and we get count out of sync.

Honza

#include <stdio.h>
volatile int idx;
struct base
{
  __attribute__((malloc,noinline))
  static void* operator new(unsigned long sz)
  {
    return ::operator new(sz);
  }

  __attribute__((malloc,noinline))
  static void operator delete(void* ptr)
  {
    --count[idx];
    ::operator delete(ptr);
  }
  volatile static int count[2];
};
volatile int base::count[2] = {0,0};
struct B:base
{
  static void* operator new(unsigned long sz)
  {
    ++count[idx];
    return base::operator new(sz);
  }

};


volatile int c=1;

int main(){
  for (int i; i<c;i++)
    {
      idx=0;
      delete new B;
      if (B::count[0] != 0)
	__builtin_abort ();
    }


  return 0;
}

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jan Hubicka]

Hi,
and this is the streaming fix

diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e4077b58890..dd9645723c1 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +647,19 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Return true if C1 and C2 are matching contexts (both translation unit decls
+   or both types.  */
+
+bool
+matching_contexts_p (tree c1, tree c2)
+{
+  if (TREE_CODE (c1) == TRANSLATION_UNIT_DECL)
+    return TREE_CODE (c2) == TRANSLATION_UNIT_DECL;
+  if (TREE_CODE (c2) == TRANSLATION_UNIT_DECL)
+    return TREE_CODE (c1) == TRANSLATION_UNIT_DECL;
+  return types_same_for_odr (c1, c2);
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +838,28 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      /* Verify that new and delete operators have the same
+			 context.  */
+		      if (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)
+			  && !matching_contexts_p
+				 (DECL_CONTEXT (def_callee),
+				  DECL_CONTEXT (gimple_call_fndecl (stmt))))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
diff --git a/gcc/tree.c b/gcc/tree.c
index 63dc6730b2b..cd608a9c878 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -5879,6 +5879,9 @@ free_lang_data_in_decl (tree decl, class free_lang_data_d *fld)
      merging may merge some fileds and keep others disjoint wich in turn will
      not do well with TREE_CHAIN pointers linking them.
 
+     tree-ssa-dce is using context to match new and delete operators (which may
+     be static functions).
+
      Also do not drop containing types for virtual methods and tables because
      these are needed by devirtualization.
      C++ destructors are special because C++ frontends sometimes produces
@@ -5886,6 +5889,9 @@ free_lang_data_in_decl (tree decl, class free_lang_data_d *fld)
      devirutalization code we always walk through aliases and we need
      context to be preserved too.  See PR89335  */
   if (TREE_CODE (decl) != FIELD_DECL
+      && (TREE_CODE (decl) != FUNCTION_DECL
+	  || (!DECL_IS_REPLACEABLE_OPERATOR_NEW_P (decl)
+	      && !DECL_IS_OPERATOR_DELETE_P (decl)))
       && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL)
           || (!DECL_VIRTUAL_P (decl)
 	      && (TREE_CODE (decl) != FUNCTION_DECL

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jan Hubicka]

Hi,
thinking a bit of the problem, I guess we could match in addition to
DECL_CONTEXT the whole inline stack of both statements and see if there
are inlined new/delete operators and if so if they are always in
matching pairs.

The inline stack is available as
for (tree block = gimple_block (call); block && TREE_CODE (block) == BLOCK; block = BLOCK_SUPERCONTEXT (block))
  {
    tree fn = block_ultimate_origin (block);
    if (fn != NULL && TREE_CODE (fn) == FUNCTION_DECL)
      do the checking htere.
  }

But I do not understand what C++ promises here and in what conditions
the new/delete pair can be removed.
Honza

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Martin Liška]

On 4/3/20 5:26 PM, Jan Hubicka wrote:
>> On 3/31/20 2:29 PM, Jan Hubicka wrote:
>>> Well, I basically went through all pointers and tried to get rid of as
>>> many of them as possible.  CONTEXT pointers do increase size of SCCs
>>> that increases chance they will not get merged and also processing time
>>> of merging algorithm.  I guess if we need to stream more contexts we
>>> could do that (and check the effect on merging and average SCC size)
>>
>> Ok, do we want to stream contexts just for the new/delete operators?
>> Can you please prepare a streaming patch?
> Hi,
> I am still not convinced that context is useful here.
> It took me a while to understand what the code does and why it fails,
> but here is a testcase.
> It fails for me with your patch and -O2 --param early-inlining-insns=100
> 
> The invalid transform is to remove pair
> base:new and B:delete
> B:new gets inlined and we get count out of sync.
> 
> Honza
> 
> #include <stdio.h>
> volatile int idx;
> struct base
> {
>    __attribute__((malloc,noinline))
>    static void* operator new(unsigned long sz)
>    {
>      return ::operator new(sz);
>    }
> 
>    __attribute__((malloc,noinline))
>    static void operator delete(void* ptr)
>    {
>      --count[idx];
>      ::operator delete(ptr);
>    }
>    volatile static int count[2];
> };
> volatile int base::count[2] = {0,0};
> struct B:base
> {
>    static void* operator new(unsigned long sz)
>    {
>      ++count[idx];
>      return base::operator new(sz);
>    }
> 
> };
> 
> 
> volatile int c=1;
> 
> int main(){
>    for (int i; i<c;i++)
>      {
>        idx=0;
>        delete new B;
>        if (B::count[0] != 0)
> 	__builtin_abort ();
>      }
> 
> 
>    return 0;
> }
> 

May I please ping Jason, Nathan and Jonathan who can help us here?

Thanks,
Martin

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Sat, Apr 4, 2020 at 1:53 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> Hi,
> thinking a bit of the problem, I guess we could match in addition to
> DECL_CONTEXT the whole inline stack of both statements and see if there
> are inlined new/delete operators and if so if they are always in
> matching pairs.
>
> The inline stack is available as
> for (tree block = gimple_block (call); block && TREE_CODE (block) == BLOCK; block = BLOCK_SUPERCONTEXT (block))
>   {
>     tree fn = block_ultimate_origin (block);
>     if (fn != NULL && TREE_CODE (fn) == FUNCTION_DECL)
>       do the checking htere.
>   }
>
> But I do not understand what C++ promises here and in what conditions
> the new/delete pair can be removed.

But if the inline stack matches in pairs then the ultimate new/delete
call should also
match, no?  When there's a mismatch in inlining we can't DCE since we
can't remove
the extra inlined stmts.

Your failing testcase suggests we never can remove new/delete pairs though
unless the DECL_CONTEXT is 'final'.  Also the user could have chosen to
"inline" the side-effect of the new operation manually but not the
delete one, so

operator delete() { count-- }

  ptr = new A;
  count++;
  delete ptr;

is it valid to elide the new/delete pair here?

Richard.

> Honza

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Nathan Sidwell]

On 4/6/20 4:34 AM, Martin Liška wrote:

> 
> May I please ping Jason, Nathan and Jonathan who can help us here?

On IRC Martin clarified the question as essentially 'how do you pair up 
operator new and operator delete calls?' (so you may delete them if the 
object is not used).

I am not sure you're permitted to remove those calls in general.  All I 
can find is [expr.new]/12
'An implementation is allowed to omit a call to a replaceable global 
allocation function (17.6.2.1, 17.6.2.2). When it does so, the storage 
is instead provided by the implementation or provided by extending the 
allocation of another new-expression.'

But, I suspect the optimization is safe, in that either no one counts 
objects by their allocation, or if they do, they don't actually care 
that the std-conforming number of allocations happen.

The both operator new and operator delete are looked up in the same 
manner.  The std does not require a 'matching pair' be found.  but it is 
extremely poor form for a class to declare exactly one of operator 
{new,delete}.

The following is well formed:

struct PoorForm {
   void *operator new (size_t s) {count++; return ::operator new (s)};
   static unsigned count;
};

Have you considered throwing ctors?

struct Obj {
   Obj (); // might throw
};

'obj = new Obj (); ... delete obj;' sequence expand to something like ...

// obj = new Obj ();
void *p = ::operator new (sizeof (Obj));
try {
   Obj::ctor(p);
}
catch (...) // cleanup code
{
   ::operator delete (p); // #1
   throw;
}

obj = (Obj*)p;

.... user code

// delete obj;
Obj::dtor (obj);
::operator delete (obj); // #2

calls 1 & 2 matchup to the operator new call

Notice that para I quoted allows one to coalesce allocations using the 
global operator new/deletes.  The rules are pretty much as you can guess 
-- one lifetime must be entirely within the other.  If inner one's ctor 
throws, the exception path must destroy the outer.

does that help?

nathan

-- 
Nathan Sidwell

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jason Merrill]

On Mon, Apr 6, 2020 at 5:27 AM Richard Biener via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> On Sat, Apr 4, 2020 at 1:53 PM Jan Hubicka <hubicka@ucw.cz> wrote:
> >
> > Hi,
> > thinking a bit of the problem, I guess we could match in addition to
> > DECL_CONTEXT the whole inline stack of both statements and see if there
> > are inlined new/delete operators and if so if they are always in
> > matching pairs.
> >
> > The inline stack is available as
> > for (tree block = gimple_block (call); block && TREE_CODE (block) ==
> BLOCK; block = BLOCK_SUPERCONTEXT (block))
> >   {
> >     tree fn = block_ultimate_origin (block);
> >     if (fn != NULL && TREE_CODE (fn) == FUNCTION_DECL)
> >       do the checking htere.
> >   }
> >
> > But I do not understand what C++ promises here and in what conditions
> > the new/delete pair can be removed.
>
> But if the inline stack matches in pairs then the ultimate new/delete
> call should also
> match, no?  When there's a mismatch in inlining we can't DCE since we
> can't remove
> the extra inlined stmts.
>
> Your failing testcase suggests we never can remove new/delete pairs though
> unless the DECL_CONTEXT is 'final'.  Also the user could have chosen to
> "inline" the side-effect of the new operation manually but not the
> delete one, so
>
> operator delete() { count-- }
>
>   ptr = new A;
>   count++;
>   delete ptr;
>
> is it valid to elide the new/delete pair here?
>

The C++ constraints are (deliberately, I think) vague.  As Nathan quotes,
it just says that a call to a replaceable operator new can be omitted, and
that if it is, the matching delete-expression should not call operator
delete.  This example seems to demonstrate that we should also only
consider the replaceable delete operators, not any operator delete.

Jason

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jonathan Wakely]

On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell <nathan@acm.org> wrote:
>
> On 4/6/20 4:34 AM, Martin Liška wrote:
>
> >
> > May I please ping Jason, Nathan and Jonathan who can help us here?
>
> On IRC Martin clarified the question as essentially 'how do you pair up
> operator new and operator delete calls?' (so you may delete them if the
> object is not used).
>
> I am not sure you're permitted to remove those calls in general.  All I
> can find is [expr.new]/12
> 'An implementation is allowed to omit a call to a replaceable global
> allocation function (17.6.2.1, 17.6.2.2). When it does so, the storage
> is instead provided by the implementation or provided by extending the
> allocation of another new-expression.'

At https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94295#c6 Richard Smith
summarised the rules as "new-expressions, like std::allocator, may
obtain storage by calling 'operator new', but it's unspecified how
often it's called and with what arguments."

So if our optimisation is removing the calls to base::operator new and
base::operator delete, but not the B::operator new call, then it seems
to be working at the wrong level. It should be eliding any calls to
operator new and operator delete at the point of the new-expression
and delete-expression, not leaving one call to operator new present
and then removing another one (which leaves the call "partially
removed").

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Tue, Apr 7, 2020 at 10:26 AM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell <nathan@acm.org> wrote:
> >
> > On 4/6/20 4:34 AM, Martin Liška wrote:
> >
> > >
> > > May I please ping Jason, Nathan and Jonathan who can help us here?
> >
> > On IRC Martin clarified the question as essentially 'how do you pair up
> > operator new and operator delete calls?' (so you may delete them if the
> > object is not used).
> >
> > I am not sure you're permitted to remove those calls in general.  All I
> > can find is [expr.new]/12
> > 'An implementation is allowed to omit a call to a replaceable global
> > allocation function (17.6.2.1, 17.6.2.2). When it does so, the storage
> > is instead provided by the implementation or provided by extending the
> > allocation of another new-expression.'
>
> At https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94295#c6 Richard Smith
> summarised the rules as "new-expressions, like std::allocator, may
> obtain storage by calling 'operator new', but it's unspecified how
> often it's called and with what arguments."
>
> So if our optimisation is removing the calls to base::operator new and
> base::operator delete, but not the B::operator new call, then it seems
> to be working at the wrong level. It should be eliding any calls to
> operator new and operator delete at the point of the new-expression
> and delete-expression, not leaving one call to operator new present
> and then removing another one (which leaves the call "partially
> removed").

Well, the question is how to identify "operator new and operator delete at the
point of the new-expression and delete-expression".  Currently we're
just matching up "is this a new/delete operator" and the dataflow of the
pointer.  In the PR it was suggested the actual called methods should have
the same DECL_CONTEXT.  Honza suggested the context should have the
"same" ODR type (or be global).

You make it sound it's much harder and the parser needs to build the
relation?  You also suggest the "validness" is only OK in the context
of std::allocator and based on the unspecifiedness of the number of
allocations from the standard library.  That would further suggest that
we need to mark the allocation/deallocation points somehow and _not_
base the optimization on the called new/delete "function" (maybe with
an exception of the global ::new and ::delete).

Richard.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jan Hubicka]

> 
> Well, the question is how to identify "operator new and operator delete at the
> point of the new-expression and delete-expression".  Currently we're
> just matching up "is this a new/delete operator" and the dataflow of the
> pointer.  In the PR it was suggested the actual called methods should have
> the same DECL_CONTEXT.  Honza suggested the context should have the
> "same" ODR type (or be global).
I was just arguing that comparing pointers to types does not make much
sence in LTO where types can get unshared :)
Since the classes have ODR name at least this problem can be solved by
using ODR name compare.

However the testcase I sent abuses the fact that if you inherit the
class you can rewrite only new operator.  In the inerited class
DECL_CONTEXT of delete will still point to the oriignal class.  This
means that you can mix new/delete pair from the original and inherited
class.

Honza
> 
> You make it sound it's much harder and the parser needs to build the
> relation?  You also suggest the "validness" is only OK in the context
> of std::allocator and based on the unspecifiedness of the number of
> allocations from the standard library.  That would further suggest that
> we need to mark the allocation/deallocation points somehow and _not_
> base the optimization on the called new/delete "function" (maybe with
> an exception of the global ::new and ::delete).
> 
> Richard.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Tue, Apr 7, 2020 at 11:49 AM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> >
> > Well, the question is how to identify "operator new and operator delete at the
> > point of the new-expression and delete-expression".  Currently we're
> > just matching up "is this a new/delete operator" and the dataflow of the
> > pointer.  In the PR it was suggested the actual called methods should have
> > the same DECL_CONTEXT.  Honza suggested the context should have the
> > "same" ODR type (or be global).
> I was just arguing that comparing pointers to types does not make much
> sence in LTO where types can get unshared :)
> Since the classes have ODR name at least this problem can be solved by
> using ODR name compare.

Sure.

> However the testcase I sent abuses the fact that if you inherit the
> class you can rewrite only new operator.  In the inerited class
> DECL_CONTEXT of delete will still point to the oriignal class.  This
> means that you can mix new/delete pair from the original and inherited
> class.

Yeah, but we're searching for a correctness fix not for an optimality one ;)

It sounds matching up the pairs in the middle-end is impossible and thus
the FE has to do this match-up (or refrain from marking new/delete when
a matchup according to to be defined methods is not going to be enough).
And maybe that tracking has to be done on a per call level rather than
on a called function level.

Richard.

> Honza
> >
> > You make it sound it's much harder and the parser needs to build the
> > relation?  You also suggest the "validness" is only OK in the context
> > of std::allocator and based on the unspecifiedness of the number of
> > allocations from the standard library.  That would further suggest that
> > we need to mark the allocation/deallocation points somehow and _not_
> > base the optimization on the called new/delete "function" (maybe with
> > an exception of the global ::new and ::delete).
> >
> > Richard.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Martin Liška]

On 4/7/20 12:22 PM, Richard Biener wrote:
> On Tue, Apr 7, 2020 at 11:49 AM Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>>>
>>> Well, the question is how to identify "operator new and operator delete at the
>>> point of the new-expression and delete-expression".  Currently we're
>>> just matching up "is this a new/delete operator" and the dataflow of the
>>> pointer.  In the PR it was suggested the actual called methods should have
>>> the same DECL_CONTEXT.  Honza suggested the context should have the
>>> "same" ODR type (or be global).
>> I was just arguing that comparing pointers to types does not make much
>> sence in LTO where types can get unshared :)
>> Since the classes have ODR name at least this problem can be solved by
>> using ODR name compare.
> 
> Sure.
> 
>> However the testcase I sent abuses the fact that if you inherit the
>> class you can rewrite only new operator.  In the inerited class
>> DECL_CONTEXT of delete will still point to the oriignal class.  This
>> means that you can mix new/delete pair from the original and inherited
>> class.
> 
> Yeah, but we're searching for a correctness fix not for an optimality one ;)
> 
> It sounds matching up the pairs in the middle-end is impossible and thus
> the FE has to do this match-up (or refrain from marking new/delete when
> a matchup according to to be defined methods is not going to be enough).
> And maybe that tracking has to be done on a per call level rather than
> on a called function level.

Based on what was said here, I see a proper matching implementation quite expensive
from implementation point of point. Moreover, the optimization itself has quite
low impact and so I suggest to only remove matching replaceable operators (1-12)
from [1], which are the top-level ones.

I'll come up with DECL_IS_REPLACEABLE_OPERATOR_DELETE_P and fix

#define DECL_IS_REPLACEABLE_OPERATOR_NEW_P(NODE) \
   (DECL_IS_OPERATOR_NEW_P (NODE) && DECL_IS_MALLOC (NODE))

which is not correct. It also matches

struct A
{
   __attribute__((noinline))
   static void* operator new(unsigned long sz)
   {
     ++count;
     return ::operator new(sz);
   }

where DECL_IS_MALLOC is discovered by local IPA passes.

Hope it's viable solution?
Thanks,
Martin

[1] https://en.cppreference.com/w/cpp/memory/new/operator_delete

> 
> Richard.
> 
>> Honza
>>>
>>> You make it sound it's much harder and the parser needs to build the
>>> relation?  You also suggest the "validness" is only OK in the context
>>> of std::allocator and based on the unspecifiedness of the number of
>>> allocations from the standard library.  That would further suggest that
>>> we need to mark the allocation/deallocation points somehow and _not_
>>> base the optimization on the called new/delete "function" (maybe with
>>> an exception of the global ::new and ::delete).
>>>
>>> Richard.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Martin Liška]

On 4/6/20 2:45 PM, Nathan Sidwell wrote:
> On 4/6/20 4:34 AM, Martin Liška wrote:
> 
>>
>> May I please ping Jason, Nathan and Jonathan who can help us here?
> 
> On IRC Martin clarified the question as essentially 'how do you pair up operator new and operator delete calls?' (so you may delete them if the object is not used).
> 
> I am not sure you're permitted to remove those calls in general.  All I can find is [expr.new]/12
> 'An implementation is allowed to omit a call to a replaceable global allocation function (17.6.2.1, 17.6.2.2). When it does so, the storage is instead provided by the implementation or provided by extending the allocation of another new-expression.'
> 
> But, I suspect the optimization is safe, in that either no one counts objects by their allocation, or if they do, they don't actually care that the std-conforming number of allocations happen.
> 
> The both operator new and operator delete are looked up in the same manner.  The std does not require a 'matching pair' be found.  but it is extremely poor form for a class to declare exactly one of operator {new,delete}.

Hi.

Thank you for the information.

> 
> The following is well formed:
> 
> struct PoorForm {
>    void *operator new (size_t s) {count++; return ::operator new (s)};
>    static unsigned count;
> };
> 
> Have you considered throwing ctors?
> 
> struct Obj {
>    Obj (); // might throw
> };
> 
> 'obj = new Obj (); ... delete obj;' sequence expand to something like ...
> 
> // obj = new Obj ();
> void *p = ::operator new (sizeof (Obj));
> try {
>    Obj::ctor(p);
> }
> catch (...) // cleanup code
> {
>    ::operator delete (p); // #1
>    throw;
> }
> 
> obj = (Obj*)p;
> 
> .... user code
> 
> // delete obj;
> Obj::dtor (obj);
> ::operator delete (obj); // #2
> 
> calls 1 & 2 matchup to the operator new call

Looking at the throwing ctors:

$ cat new-delete2.C
#include <stdio.h>

struct A
{
   __attribute__((always_inline)) A(int x)
   {
     if (x == 123)
       throw x;
   }
};

int main(int argc, char **argv) {
   A *a = new A (argc);
   delete a;

   return 0;
}

$ g++-9 new-delete2.C -O2 -c -fdump-tree-optimized=/dev/stdout
...
   <bb 2> [local count: 1073741824]:
   _3 = operator new (1);
   if (argc_4(D) == 123)
     goto <bb 3>; [0.00%]
   else
     goto <bb 4>; [100.00%]

   <bb 3> [count: 0]:
   _8 = __cxa_allocate_exception (4);
   MEM[(int *)_8] = 123;
   __cxa_throw (_8, &_ZTIi, 0B);

   <bb 4> [local count: 1073741824]:
   operator delete (_3, 1);
   return 0;
...

As seen cddce can still optimize out
   _3 = operator new (1);
...
   operator delete (_3, 1);

Martin

> 
> Notice that para I quoted allows one to coalesce allocations using the global operator new/deletes.  The rules are pretty much as you can guess -- one lifetime must be entirely within the other.  If inner one's ctor throws, the exception path must destroy the outer.
> 
> does that help?
> 
> nathan
> 

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jonathan Wakely]

On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
> The both operator new and operator delete are looked up in the same
> manner.  The std does not require a 'matching pair' be found.  but it is
> extremely poor form for a class to declare exactly one of operator
> {new,delete}.

There are unfortunately several such example in the standard!

I wonder how much benefit we will really get from trying to make this
optimisation too general.

Just eliding (or coalescing) implicit calls to ::operator new(size_t)
and ::operator delete(void*, size_t) (and the [] and align_val_t forms
of those) probably covers 99% of real cases.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Tue, Apr 7, 2020 at 1:30 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
> > The both operator new and operator delete are looked up in the same
> > manner.  The std does not require a 'matching pair' be found.  but it is
> > extremely poor form for a class to declare exactly one of operator
> > {new,delete}.
>
> There are unfortunately several such example in the standard!
>
> I wonder how much benefit we will really get from trying to make this
> optimisation too general.
>
> Just eliding (or coalescing) implicit calls to ::operator new(size_t)
> and ::operator delete(void*, size_t) (and the [] and align_val_t forms
> of those) probably covers 99% of real cases.

IIRC the only reason to have the optimization was to fully elide
STL containers when possible.  That brings in allocators and
thus non ::new/::delete allocations.  Which then suggests that
our standard library implementation could annotate allocation
points somehow.

Richard.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jonathan Wakely]

On Tue, 7 Apr 2020 at 10:29, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 10:26 AM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell <nathan@acm.org> wrote:
> > >
> > > On 4/6/20 4:34 AM, Martin Liška wrote:
> > >
> > > >
> > > > May I please ping Jason, Nathan and Jonathan who can help us here?
> > >
> > > On IRC Martin clarified the question as essentially 'how do you pair up
> > > operator new and operator delete calls?' (so you may delete them if the
> > > object is not used).
> > >
> > > I am not sure you're permitted to remove those calls in general.  All I
> > > can find is [expr.new]/12
> > > 'An implementation is allowed to omit a call to a replaceable global
> > > allocation function (17.6.2.1, 17.6.2.2). When it does so, the storage
> > > is instead provided by the implementation or provided by extending the
> > > allocation of another new-expression.'
> >
> > At https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94295#c6 Richard Smith
> > summarised the rules as "new-expressions, like std::allocator, may
> > obtain storage by calling 'operator new', but it's unspecified how
> > often it's called and with what arguments."
> >
> > So if our optimisation is removing the calls to base::operator new and
> > base::operator delete, but not the B::operator new call, then it seems
> > to be working at the wrong level. It should be eliding any calls to
> > operator new and operator delete at the point of the new-expression
> > and delete-expression, not leaving one call to operator new present
> > and then removing another one (which leaves the call "partially
> > removed").
>
> Well, the question is how to identify "operator new and operator delete at the
> point of the new-expression and delete-expression".  Currently we're
> just matching up "is this a new/delete operator" and the dataflow of the
> pointer.  In the PR it was suggested the actual called methods should have
> the same DECL_CONTEXT.  Honza suggested the context should have the
> "same" ODR type (or be global).
>
> You make it sound it's much harder and the parser needs to build the
> relation?  You also suggest the "validness" is only OK in the context
> of std::allocator and based on the unspecifiedness of the number of
> allocations from the standard library.

I don't think Richard's summary or my paraphrasing intends to say it
only applies to std::allocator.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jonathan Wakely]

On Tue, 7 Apr 2020 at 12:40, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 1:30 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
> > > The both operator new and operator delete are looked up in the same
> > > manner.  The std does not require a 'matching pair' be found.  but it is
> > > extremely poor form for a class to declare exactly one of operator
> > > {new,delete}.
> >
> > There are unfortunately several such example in the standard!
> >
> > I wonder how much benefit we will really get from trying to make this
> > optimisation too general.
> >
> > Just eliding (or coalescing) implicit calls to ::operator new(size_t)
> > and ::operator delete(void*, size_t) (and the [] and align_val_t forms
> > of those) probably covers 99% of real cases.
>
> IIRC the only reason to have the optimization was to fully elide
> STL containers when possible.  That brings in allocators and
> thus non ::new/::delete allocations.

But the vast majority of containers are used with std::allocator, and
we control that.

Wouldn't it be simpler to add __builtin_operator_new and
__builtin_operator_delete like clang has, then make std::allocator use
those, and limit the optimizations to uses of those built-ins?

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Tue, Apr 7, 2020 at 1:46 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 7 Apr 2020 at 12:40, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Apr 7, 2020 at 1:30 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > >
> > > On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
> > > > The both operator new and operator delete are looked up in the same
> > > > manner.  The std does not require a 'matching pair' be found.  but it is
> > > > extremely poor form for a class to declare exactly one of operator
> > > > {new,delete}.
> > >
> > > There are unfortunately several such example in the standard!
> > >
> > > I wonder how much benefit we will really get from trying to make this
> > > optimisation too general.
> > >
> > > Just eliding (or coalescing) implicit calls to ::operator new(size_t)
> > > and ::operator delete(void*, size_t) (and the [] and align_val_t forms
> > > of those) probably covers 99% of real cases.
> >
> > IIRC the only reason to have the optimization was to fully elide
> > STL containers when possible.  That brings in allocators and
> > thus non ::new/::delete allocations.
>
> But the vast majority of containers are used with std::allocator, and
> we control that.
>
> Wouldn't it be simpler to add __builtin_operator_new and
> __builtin_operator_delete like clang has, then make std::allocator use
> those, and limit the optimizations to uses of those built-ins?

Sure, that's a viable implementation strathegy.  Another might be

void delete (void *) __attribute__((matching_new(somewhere::new)));

and thus allow the user to relate a new/delete pair (here the FE would
do lookup for 'new' and record for example the mangled assembler name).

That is, we usually try to design a mechanism that's more broadly usable.
But yes, we match malloc/free so matching ::new/::delete by aliasing them
to __builtin_operator_new and __builtin_operator_delete is fair enough.

Not easily usable by other languages with custom allocation though
(fortran allocate/deallocate anyone? that's currently inlined to expose
underlying malloc/free calls for similar reasons)

Richard.

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Nathan Sidwell]

On 4/7/20 7:29 AM, Jonathan Wakely wrote:
> On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
>> The both operator new and operator delete are looked up in the same
>> manner.  The std does not require a 'matching pair' be found.  but it is
>> extremely poor form for a class to declare exactly one of operator
>> {new,delete}.
> 
> There are unfortunately several such example in the standard!

Pah!

I also realized one could write:

struct Derived : Base {
   void *operator new (size_t t) { ... }
   using Base::operator delete;
};

which is not such poor code, but the FE will generate calls that 
completely lose the usingness of the operator delete.

As RichardB comments, I think the FE needs to mark sets of new/delete 
calls for the middle end to safely optimize. but ...

> 
> I wonder how much benefit we will really get from trying to make this
> optimisation too general.
> 
> Just eliding (or coalescing) implicit calls to ::operator new(size_t)
> and ::operator delete(void*, size_t) (and the [] and align_val_t forms
> of those) probably covers 99% of real cases.

I agree.  It's certainly a simpler problem and the major component of 
any win we'll get.

nathan

-- 
Nathan Sidwell

[PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

Hi.

The patch allows DCE to remove only replaceable operators new and delete.
That's achieved by proper mark up of all these operators in C++ FE.
The patch also brings all tests we've collected so far for the PR.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-Allow-new-delete-operator-deletion-only-for-replacea.patch --]
[-- Type: text/x-patch, Size: 16355 bytes --]

From 815495bbd52abe0da173c8f88d1de8dcb6095ba7 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 7 Apr 2020 16:23:27 +0200
Subject: [PATCH] Allow new/delete operator deletion only for replaceable.

gcc/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* gimple.c (gimple_call_operator_delete_p): Rename to...
	(gimple_call_replaceable_operator_delete_p): ... this.
	Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
	* gimple.h (gimple_call_operator_delete_p): Rename to ...
	(gimple_call_replaceable_operator_delete_p): ... this.
	* tree-core.h (tree_function_decl): Add replaceable_operator
	flag.
	* tree-ssa-dce.c (mark_all_reaching_defs_necessary_1):
	Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
	(propagate_necessity): Use gimple_call_replaceable_operator_delete_p.
	(eliminate_unnecessary_stmts): Likewise.
	* tree-streamer-in.c (unpack_ts_function_decl_value_fields):
	Pack DECL_IS_REPLACEABLE_OPERATOR.
	* tree-streamer-out.c (pack_ts_function_decl_value_fields):
	Unpack the field here.
	* tree.h (DECL_IS_REPLACEABLE_OPERATOR): New.
	(DECL_IS_REPLACEABLE_OPERATOR_NEW_P): New.
	(DECL_IS_REPLACEABLE_OPERATOR_DELETE_P): New.

gcc/cp/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* decl.c (duplicate_decls): Duplicate also DECL_IS_REPLACEABLE_OPERATOR.
	(cxx_init_decl_processing): Mark replaceable all implicitly defined
	operators.

gcc/lto/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* lto-common.c (compare_tree_sccs_1): Compare also
	DECL_IS_REPLACEABLE_OPERATOR.

gcc/testsuite/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* g++.dg/pr94314-2.C: New test.
	* g++.dg/pr94314-3.C: New test.
	* g++.dg/pr94314.C: New test.
---
 gcc/cp/decl.c                    | 14 ++++++
 gcc/gimple.c                     |  6 +--
 gcc/gimple.h                     |  2 +-
 gcc/lto/lto-common.c             |  1 +
 gcc/testsuite/g++.dg/pr94314-2.C | 26 ++++++++++
 gcc/testsuite/g++.dg/pr94314-3.C | 55 +++++++++++++++++++++
 gcc/testsuite/g++.dg/pr94314.C   | 85 ++++++++++++++++++++++++++++++++
 gcc/tree-core.h                  |  3 +-
 gcc/tree-ssa-dce.c               |  8 +--
 gcc/tree-streamer-in.c           |  1 +
 gcc/tree-streamer-out.c          |  1 +
 gcc/tree.h                       | 10 +++-
 12 files changed, 202 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-2.C
 create mode 100644 gcc/testsuite/g++.dg/pr94314-3.C
 create mode 100644 gcc/testsuite/g++.dg/pr94314.C

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index a127734af69..a731702bdd2 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2368,6 +2368,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
 	    DECL_SET_IS_OPERATOR_NEW (newdecl, true);
 	  DECL_LOOPING_CONST_OR_PURE_P (newdecl)
 	    |= DECL_LOOPING_CONST_OR_PURE_P (olddecl);
+	  DECL_IS_REPLACEABLE_OPERATOR (newdecl)
+	    |= DECL_IS_REPLACEABLE_OPERATOR (olddecl);
 
 	  if (merge_attr)
 	    merge_attribute_bits (newdecl, olddecl);
@@ -4438,13 +4440,17 @@ cxx_init_decl_processing (void)
     tree opnew = push_cp_library_fn (NEW_EXPR, newtype, 0);
     DECL_IS_MALLOC (opnew) = 1;
     DECL_SET_IS_OPERATOR_NEW (opnew, true);
+    DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
     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;
     opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
     DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
     if (flag_sized_deallocation)
       {
 	/* Also push the sized deallocation variants:
@@ -4458,8 +4464,10 @@ cxx_init_decl_processing (void)
 	deltype = build_exception_variant (deltype, empty_except_spec);
 	opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
       }
 
     if (aligned_new_threshold)
@@ -4478,9 +4486,11 @@ cxx_init_decl_processing (void)
 	opnew = push_cp_library_fn (NEW_EXPR, newtype, 0);
 	DECL_IS_MALLOC (opnew) = 1;
 	DECL_SET_IS_OPERATOR_NEW (opnew, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
 	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;
 
 	/* operator delete (void *, align_val_t); */
 	deltype = build_function_type_list (void_type_node, ptr_type_node,
@@ -4489,8 +4499,10 @@ cxx_init_decl_processing (void)
 	deltype = build_exception_variant (deltype, empty_except_spec);
 	opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 
 	if (flag_sized_deallocation)
 	  {
@@ -4502,8 +4514,10 @@ cxx_init_decl_processing (void)
 	    deltype = build_exception_variant (deltype, empty_except_spec);
 	    opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	    DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	    opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	    DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	  }
       }
 
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 61a400b356f..10c562f4def 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2730,15 +2730,15 @@ gimple_builtin_call_types_compatible_p (const gimple *stmt, tree fndecl)
   return true;
 }
 
-/* Return true when STMT is operator delete call.  */
+/* Return true when STMT is operator a replaceable delete call.  */
 
 bool
-gimple_call_operator_delete_p (const gcall *stmt)
+gimple_call_replaceable_operator_delete_p (const gcall *stmt)
 {
   tree fndecl;
 
   if ((fndecl = gimple_call_fndecl (stmt)) != NULL_TREE)
-    return DECL_IS_OPERATOR_DELETE_P (fndecl);
+    return DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (fndecl);
   return false;
 }
 
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 305d98f5438..ca7fec6247e 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1615,7 +1615,7 @@ extern alias_set_type gimple_get_alias_set (tree);
 extern bool gimple_ior_addresses_taken (bitmap, gimple *);
 extern bool gimple_builtin_call_types_compatible_p (const gimple *, tree);
 extern combined_fn gimple_call_combined_fn (const gimple *);
-extern bool gimple_call_operator_delete_p (const gcall *);
+extern bool gimple_call_replaceable_operator_delete_p (const gcall *);
 extern bool gimple_call_builtin_p (const gimple *);
 extern bool gimple_call_builtin_p (const gimple *, enum built_in_class);
 extern bool gimple_call_builtin_p (const gimple *, enum built_in_function);
diff --git a/gcc/lto/lto-common.c b/gcc/lto/lto-common.c
index c95a9b00d9e..e073abce2e7 100644
--- a/gcc/lto/lto-common.c
+++ b/gcc/lto/lto-common.c
@@ -1236,6 +1236,7 @@ compare_tree_sccs_1 (tree t1, tree t2, tree **map)
       compare_values (DECL_DISREGARD_INLINE_LIMITS);
       compare_values (DECL_PURE_P);
       compare_values (DECL_LOOPING_CONST_OR_PURE_P);
+      compare_values (DECL_IS_REPLACEABLE_OPERATOR);
       compare_values (DECL_FINAL_P);
       compare_values (DECL_CXX_CONSTRUCTOR_P);
       compare_values (DECL_CXX_DESTRUCTOR_P);
diff --git a/gcc/testsuite/g++.dg/pr94314-2.C b/gcc/testsuite/g++.dg/pr94314-2.C
new file mode 100644
index 00000000000..36b93ed6d4d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-2.C
@@ -0,0 +1,26 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+struct A
+{
+  __attribute__((always_inline)) A(int x)
+  {
+    if (x == 123)
+      throw x;
+  }
+};
+
+int
+main(int argc, char **argv)
+{
+  A *a = new A (argc);
+  delete a;
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 2 "cddce1"} } */
diff --git a/gcc/testsuite/g++.dg/pr94314-3.C b/gcc/testsuite/g++.dg/pr94314-3.C
new file mode 100644
index 00000000000..a5b10139290
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-3.C
@@ -0,0 +1,55 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 --param early-inlining-insns=100 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+volatile int idx;
+
+struct base
+{
+  __attribute__ ((malloc, noinline)) static void *
+  operator new (unsigned long sz)
+  {
+    return ::operator new (sz);
+  }
+
+  __attribute__ ((noinline)) static void operator delete (void *ptr)
+  {
+    int c = count[idx];
+    count[idx] = c - 1;
+    ::operator delete (ptr);
+  }
+  volatile static int count[2];
+};
+
+volatile int base::count[2] = {0, 0};
+
+struct B : base
+{
+  static void *operator new (unsigned long sz)
+  {
+    int c = count[idx];
+    count[idx] = c + 1;
+    return base::operator new (sz);
+  }
+};
+
+volatile int c = 1;
+
+int
+main ()
+{
+  for (int i; i < c; i++)
+    {
+      idx = 0;
+      delete new B;
+      if (B::count[0] != 0)
+	__builtin_abort ();
+    }
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/testsuite/g++.dg/pr94314.C b/gcc/testsuite/g++.dg/pr94314.C
new file mode 100644
index 00000000000..a06800d2219
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314.C
@@ -0,0 +1,85 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+struct A
+{
+  __attribute__((malloc,noinline))
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int A::count = 0;
+
+struct B
+{
+  __attribute__((malloc,noinline))
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  __attribute__((noinline))
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int B::count = 0;
+
+struct C
+{
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int C::count = 0;
+
+int main(){
+  delete new A;
+  if (A::count != 0)
+    __builtin_abort ();
+
+  delete new B;
+  if (B::count != 0)
+    __builtin_abort ();
+
+  delete new C;
+  if (C::count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 1 "cddce1"} } */
+/* { dg-final { scan-tree-dump-not "Deleting : B::operator delete" "cddce1"} } */
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index 765ea2a9542..d84fe959acc 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1896,8 +1896,9 @@ struct GTY(()) tree_function_decl {
   ENUM_BITFIELD(function_decl_type) decl_type: 2;
   unsigned has_debug_args_flag : 1;
   unsigned versioned_function : 1;
+  unsigned replaceable_operator : 1;
 
-  /* 12 bits left for future expansion.  */
+  /* 11 bits left for future expansion.  */
 };
 
 struct GTY(()) tree_translation_unit_decl {
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e4077b58890..fd5f24c746c 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -614,7 +614,7 @@ mark_all_reaching_defs_necessary_1 (ao_ref *ref ATTRIBUTE_UNUSED,
 
       if (callee != NULL_TREE
 	  && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee)
-	      || DECL_IS_OPERATOR_DELETE_P (callee)))
+	      || DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (callee)))
 	return false;
     }
 
@@ -806,7 +806,7 @@ propagate_necessity (bool aggressive)
 	     processing the argument.  */
 	  bool is_delete_operator
 	    = (is_gimple_call (stmt)
-	       && gimple_call_operator_delete_p (as_a <gcall *> (stmt)));
+	       && gimple_call_replaceable_operator_delete_p (as_a <gcall *> (stmt)));
 	  if (is_delete_operator
 	      || gimple_call_builtin_p (stmt, BUILT_IN_FREE))
 	    {
@@ -896,7 +896,7 @@ propagate_necessity (bool aggressive)
 
 	      if (callee != NULL_TREE
 		  && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee)
-		      || DECL_IS_OPERATOR_DELETE_P (callee)))
+		      || DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (callee)))
 		continue;
 
 	      /* Calls implicitly load from memory, their arguments
@@ -1321,7 +1321,7 @@ eliminate_unnecessary_stmts (void)
 	  if (gimple_plf (stmt, STMT_NECESSARY)
 	      && (gimple_call_builtin_p (stmt, BUILT_IN_FREE)
 		  || (is_gimple_call (stmt)
-		      && gimple_call_operator_delete_p (as_a <gcall *> (stmt)))))
+		      && gimple_call_replaceable_operator_delete_p (as_a <gcall *> (stmt)))))
 	    {
 	      tree ptr = gimple_call_arg (stmt, 0);
 	      if (TREE_CODE (ptr) == SSA_NAME)
diff --git a/gcc/tree-streamer-in.c b/gcc/tree-streamer-in.c
index 4e035e9638f..0bfc272d076 100644
--- a/gcc/tree-streamer-in.c
+++ b/gcc/tree-streamer-in.c
@@ -343,6 +343,7 @@ unpack_ts_function_decl_value_fields (struct bitpack_d *bp, tree expr)
   DECL_DISREGARD_INLINE_LIMITS (expr) = (unsigned) bp_unpack_value (bp, 1);
   DECL_PURE_P (expr) = (unsigned) bp_unpack_value (bp, 1);
   DECL_LOOPING_CONST_OR_PURE_P (expr) = (unsigned) bp_unpack_value (bp, 1);
+  DECL_IS_REPLACEABLE_OPERATOR (expr) = (unsigned) bp_unpack_value (bp, 1);
   unsigned int fcode = 0;
   if (cl != NOT_BUILT_IN)
     {
diff --git a/gcc/tree-streamer-out.c b/gcc/tree-streamer-out.c
index 8e5e1355196..5bbcebba87e 100644
--- a/gcc/tree-streamer-out.c
+++ b/gcc/tree-streamer-out.c
@@ -305,6 +305,7 @@ pack_ts_function_decl_value_fields (struct bitpack_d *bp, tree expr)
   bp_pack_value (bp, DECL_DISREGARD_INLINE_LIMITS (expr), 1);
   bp_pack_value (bp, DECL_PURE_P (expr), 1);
   bp_pack_value (bp, DECL_LOOPING_CONST_OR_PURE_P (expr), 1);
+  bp_pack_value (bp, DECL_IS_REPLACEABLE_OPERATOR (expr), 1);
   if (DECL_BUILT_IN_CLASS (expr) != NOT_BUILT_IN)
     bp_pack_value (bp, DECL_UNCHECKED_FUNCTION_CODE (expr), 32);
 }
diff --git a/gcc/tree.h b/gcc/tree.h
index 66dfa876256..1c28785d411 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3037,6 +3037,11 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
     FUNCTION_DECL_DECL_TYPE (decl) = NONE;
 }
 
+/* Nonzero in a FUNCTION_DECL means this function is a replaceable
+   function (like replaceable operators new or delete).  */
+#define DECL_IS_REPLACEABLE_OPERATOR(NODE)\
+   (FUNCTION_DECL_CHECK (NODE)->function_decl.replaceable_operator)
+
 /* Nonzero in a FUNCTION_DECL means this function should be treated as
    C++ operator new, meaning that it returns a pointer for which we
    should not use type based aliasing.  */
@@ -3044,7 +3049,7 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
   (FUNCTION_DECL_CHECK (NODE)->function_decl.decl_type == OPERATOR_NEW)
 
 #define DECL_IS_REPLACEABLE_OPERATOR_NEW_P(NODE) \
-  (DECL_IS_OPERATOR_NEW_P (NODE) && DECL_IS_MALLOC (NODE))
+  (DECL_IS_OPERATOR_NEW_P (NODE) && DECL_IS_REPLACEABLE_OPERATOR (NODE))
 
 #define DECL_SET_IS_OPERATOR_NEW(NODE, VAL) \
   set_function_decl_type (FUNCTION_DECL_CHECK (NODE), OPERATOR_NEW, VAL)
@@ -3054,6 +3059,9 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
 #define DECL_IS_OPERATOR_DELETE_P(NODE) \
   (FUNCTION_DECL_CHECK (NODE)->function_decl.decl_type == OPERATOR_DELETE)
 
+#define DECL_IS_REPLACEABLE_OPERATOR_DELETE_P(NODE) \
+  (DECL_IS_OPERATOR_DELETE_P (NODE) && DECL_IS_REPLACEABLE_OPERATOR (NODE))
+
 #define DECL_SET_IS_OPERATOR_DELETE(NODE, VAL) \
   set_function_decl_type (FUNCTION_DECL_CHECK (NODE), OPERATOR_DELETE, VAL)
 
-- 
2.26.0

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jonathan Wakely]

On Tue, 7 Apr 2020 at 12:57, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 1:46 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Tue, 7 Apr 2020 at 12:40, Richard Biener <richard.guenther@gmail.com> wrote:
> > >
> > > On Tue, Apr 7, 2020 at 1:30 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > > >
> > > > On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
> > > > > The both operator new and operator delete are looked up in the same
> > > > > manner.  The std does not require a 'matching pair' be found.  but it is
> > > > > extremely poor form for a class to declare exactly one of operator
> > > > > {new,delete}.
> > > >
> > > > There are unfortunately several such example in the standard!
> > > >
> > > > I wonder how much benefit we will really get from trying to make this
> > > > optimisation too general.
> > > >
> > > > Just eliding (or coalescing) implicit calls to ::operator new(size_t)
> > > > and ::operator delete(void*, size_t) (and the [] and align_val_t forms
> > > > of those) probably covers 99% of real cases.
> > >
> > > IIRC the only reason to have the optimization was to fully elide
> > > STL containers when possible.  That brings in allocators and
> > > thus non ::new/::delete allocations.
> >
> > But the vast majority of containers are used with std::allocator, and
> > we control that.
> >
> > Wouldn't it be simpler to add __builtin_operator_new and
> > __builtin_operator_delete like clang has, then make std::allocator use
> > those, and limit the optimizations to uses of those built-ins?
>
> Sure, that's a viable implementation strathegy.  Another might be
>
> void delete (void *) __attribute__((matching_new(somewhere::new)));
>
> and thus allow the user to relate a new/delete pair (here the FE would
> do lookup for 'new' and record for example the mangled assembler name).

Does that attribute tell us anything useful?

Given a pointer obtained from new and passed to delete, can't we
assume they are a matching pair? If not, the behaviour would be
undefined anyway.

> That is, we usually try to design a mechanism that's more broadly usable.
> But yes, we match malloc/free so matching ::new/::delete by aliasing them
> to __builtin_operator_new and __builtin_operator_delete is fair enough.

Would it make sense to annotate the actual calls to the
allocation/deallocation functions, instead of the declarations of
those functions?

According to Richard Smith, we don't want to optimise away 'operator
delete(operator new(1), 1)' because that's an explicit call, and the
user might have replaced those functions and be relying on the side
effects. But we can optimise away 'delete new char' and we can
optimise away std::allocator<char>().deallocate(std::allocator<char>().allocate(1),
1). So what matters is not whether the functions being called match up
(they have to anyway) or which functions are being called. What
matters is whether they are called implicitly (by a new-expression or
by std::allocator).

So when the compiler expands 'new T' into a call to operator new
followed by constructing a T (plus exception handling) the call to
operator new would be marked as optimisable by the FE, and likewise
when the compiler expands 'delete p' into a destructor followed by
calling operator delete, the call to operator delete would be marked
as optimisable. If a pointer is allocated by a call marked optimisable
and deallocated by a call marked optimisable, it can be optimised away
(or coalesced with another optimisation).

Then for std::allocator we just want to be able to mark the explicit
calls to operator new and operator delete as "optimisable", as the FE
does for the implicit calls it generates. So if we want a general
purpose utility, it would be an attribute to mark /calls/ as
optimisable, not functions.

Adding __builtin_operator_new would just be a different syntax for
"call operator new but mark it optimisable".

N.B. I am not a compiler dev and might be talking nonsense :-)

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Richard Biener]

On Tue, Apr 7, 2020 at 5:17 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 7 Apr 2020 at 12:57, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Apr 7, 2020 at 1:46 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > >
> > > On Tue, 7 Apr 2020 at 12:40, Richard Biener <richard.guenther@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 7, 2020 at 1:30 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > > > >
> > > > > On Mon, 6 Apr 2020 at 13:45, Nathan Sidwell wrote:
> > > > > > The both operator new and operator delete are looked up in the same
> > > > > > manner.  The std does not require a 'matching pair' be found.  but it is
> > > > > > extremely poor form for a class to declare exactly one of operator
> > > > > > {new,delete}.
> > > > >
> > > > > There are unfortunately several such example in the standard!
> > > > >
> > > > > I wonder how much benefit we will really get from trying to make this
> > > > > optimisation too general.
> > > > >
> > > > > Just eliding (or coalescing) implicit calls to ::operator new(size_t)
> > > > > and ::operator delete(void*, size_t) (and the [] and align_val_t forms
> > > > > of those) probably covers 99% of real cases.
> > > >
> > > > IIRC the only reason to have the optimization was to fully elide
> > > > STL containers when possible.  That brings in allocators and
> > > > thus non ::new/::delete allocations.
> > >
> > > But the vast majority of containers are used with std::allocator, and
> > > we control that.
> > >
> > > Wouldn't it be simpler to add __builtin_operator_new and
> > > __builtin_operator_delete like clang has, then make std::allocator use
> > > those, and limit the optimizations to uses of those built-ins?
> >
> > Sure, that's a viable implementation strathegy.  Another might be
> >
> > void delete (void *) __attribute__((matching_new(somewhere::new)));
> >
> > and thus allow the user to relate a new/delete pair (here the FE would
> > do lookup for 'new' and record for example the mangled assembler name).
>
> Does that attribute tell us anything useful?
>
> Given a pointer obtained from new and passed to delete, can't we
> assume they are a matching pair? If not, the behaviour would be
> undefined anyway.

Further matching of new/delete came up in the context of inlining where
we might not be able to elide side-effects of new/delete "appropriately".
That's actually the case in the referenced PR.

> > That is, we usually try to design a mechanism that's more broadly usable.
> > But yes, we match malloc/free so matching ::new/::delete by aliasing them
> > to __builtin_operator_new and __builtin_operator_delete is fair enough.
>
> Would it make sense to annotate the actual calls to the
> allocation/deallocation functions, instead of the declarations of
> those functions?

Sure - I think that's ultimatively the way to go.

> According to Richard Smith, we don't want to optimise away 'operator
> delete(operator new(1), 1)' because that's an explicit call, and the
> user might have replaced those functions and be relying on the side
> effects. But we can optimise away 'delete new char' and we can
> optimise away std::allocator<char>().deallocate(std::allocator<char>().allocate(1),
> 1). So what matters is not whether the functions being called match up
> (they have to anyway) or which functions are being called. What
> matters is whether they are called implicitly (by a new-expression or
> by std::allocator).

OK, I see.  So for the inline case we'd for example have

operator new() { return ::new ...; }

and

 delete new T;

where the generated and marked operator new is inlined we'd no longer
elide the pair because the operator new implementation had an explicit
call to ::new and this is what the optimization sees now.  Only when
the operator new implementation uses a new expression again we'd
see it as candidate pair and then run into the inline issue again
(the issue of the new operator implementation incrementing some counter
and the delete one decrementing it, us inlining one of both and then
eliding the pair, only eliding either the increment or the decrement
and thus retaining half of the overall side-effect).

> So when the compiler expands 'new T' into a call to operator new
> followed by constructing a T (plus exception handling) the call to
> operator new would be marked as optimisable by the FE, and likewise
> when the compiler expands 'delete p' into a destructor followed by
> calling operator delete, the call to operator delete would be marked
> as optimisable. If a pointer is allocated by a call marked optimisable
> and deallocated by a call marked optimisable, it can be optimised away
> (or coalesced with another optimisation).
>
> Then for std::allocator we just want to be able to mark the explicit
> calls to operator new and operator delete as "optimisable", as the FE
> does for the implicit calls it generates. So if we want a general
> purpose utility, it would be an attribute to mark /calls/ as
> optimisable, not functions.
>
> Adding __builtin_operator_new would just be a different syntax for
> "call operator new but mark it optimisable".

Ah, so __builtin_operator_new isn't a function but an alternate
new expression syntax?

Richard.

> N.B. I am not a compiler dev and might be talking nonsense :-)

Re: [PATCH] Check DECL_CONTEXT of new/delete operators.

[Jonathan Wakely]

On Wed, 8 Apr 2020 at 08:35, Richard Biener wrote:
> Ah, so __builtin_operator_new isn't a function but an alternate
> new expression syntax?

No, not a new-expression, because a new-expression calls operator new
to obtain storage *and* begins the lifetime of one or more objects in
that storage. __builtin_operator_new is just the first part, i.e. the
operator new(...) call. But because explicit calls to operator
new(...) are not supposed to be optimized, __builtin_operator_new is a
way of calling operator new(...) that tells the compiler "this isn't
an explicit call, you can optimise it". So a new-expression can use it
(because that needs to call operator new(...), but the call should be
optimisable) and std::allocator<T>::allocate(n) can use it (because
that call is also optimisable).

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Richard Biener]

On Tue, Apr 7, 2020 at 5:01 PM Martin Liška <mliska@suse.cz> wrote:
>
> Hi.
>
> The patch allows DCE to remove only replaceable operators new and delete.
> That's achieved by proper mark up of all these operators in C++ FE.
> The patch also brings all tests we've collected so far for the PR.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

Grepping for uses of DECL_IS_OPERATOR_* reveals you miss comparing
the new flag in ipa-icf.c and cgraph node dumping in cgraph.c might want
to dump it as well.

Otherwise it looks reasonable.

So the mid-end parts are OK in case FE people are happy with this solution
for GCC 10.

Richard.

> Thanks,
> Martin

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jason Merrill]

On 4/8/20 4:47 AM, Richard Biener wrote:
> On Tue, Apr 7, 2020 at 5:01 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> Hi.
>>
>> The patch allows DCE to remove only replaceable operators new and delete.
>> That's achieved by proper mark up of all these operators in C++ FE.
>> The patch also brings all tests we've collected so far for the PR.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> Grepping for uses of DECL_IS_OPERATOR_* reveals you miss comparing
> the new flag in ipa-icf.c and cgraph node dumping in cgraph.c might want
> to dump it as well.
> 
> Otherwise it looks reasonable.
> 
> So the mid-end parts are OK in case FE people are happy with this solution
> for GCC 10.

This seems fine for GCC 10, though I wonder about using an attribute for 
DECL_REPLACEABLE_OPERATOR rather than taking a bit in all FUNCTION_DECLs 
that will only ever be set on a small handful.

For GCC 11 we probably want to make the distinction Jonathan mentions 
between new-expressions and direct calls to operator new.

Jason

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jakub Jelinek]

On Wed, Apr 08, 2020 at 09:20:07AM -0400, Jason Merrill via Gcc-patches wrote:
> On 4/8/20 4:47 AM, Richard Biener wrote:
> > On Tue, Apr 7, 2020 at 5:01 PM Martin Liška <mliska@suse.cz> wrote:
> > > 
> > > Hi.
> > > 
> > > The patch allows DCE to remove only replaceable operators new and delete.
> > > That's achieved by proper mark up of all these operators in C++ FE.
> > > The patch also brings all tests we've collected so far for the PR.
> > > 
> > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > > 
> > > Ready to be installed?
> > 
> > Grepping for uses of DECL_IS_OPERATOR_* reveals you miss comparing
> > the new flag in ipa-icf.c and cgraph node dumping in cgraph.c might want
> > to dump it as well.
> > 
> > Otherwise it looks reasonable.
> > 
> > So the mid-end parts are OK in case FE people are happy with this solution
> > for GCC 10.
> 
> This seems fine for GCC 10, though I wonder about using an attribute for
> DECL_REPLACEABLE_OPERATOR rather than taking a bit in all FUNCTION_DECLs
> that will only ever be set on a small handful.

If it is just for GCC 10 and we'll return the bit back to unused in GCC 11,
I'd think it is acceptable.

> For GCC 11 we probably want to make the distinction Jonathan mentions
> between new-expressions and direct calls to operator new.

Yeah.

	Jakub

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jason Merrill]

On 4/8/20 9:32 AM, Jakub Jelinek wrote:
> On Wed, Apr 08, 2020 at 09:20:07AM -0400, Jason Merrill via Gcc-patches wrote:
>> On 4/8/20 4:47 AM, Richard Biener wrote:
>>> On Tue, Apr 7, 2020 at 5:01 PM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> Hi.
>>>>
>>>> The patch allows DCE to remove only replaceable operators new and delete.
>>>> That's achieved by proper mark up of all these operators in C++ FE.
>>>> The patch also brings all tests we've collected so far for the PR.
>>>>
>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>
>>> Grepping for uses of DECL_IS_OPERATOR_* reveals you miss comparing
>>> the new flag in ipa-icf.c and cgraph node dumping in cgraph.c might want
>>> to dump it as well.
>>>
>>> Otherwise it looks reasonable.
>>>
>>> So the mid-end parts are OK in case FE people are happy with this solution
>>> for GCC 10.
>>
>> This seems fine for GCC 10, though I wonder about using an attribute for
>> DECL_REPLACEABLE_OPERATOR rather than taking a bit in all FUNCTION_DECLs
>> that will only ever be set on a small handful.
> 
> If it is just for GCC 10 and we'll return the bit back to unused in GCC 11,
> I'd think it is acceptable.

Agreed.

Jason

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/8/20 3:34 PM, Jason Merrill wrote:
> On 4/8/20 9:32 AM, Jakub Jelinek wrote:
>> On Wed, Apr 08, 2020 at 09:20:07AM -0400, Jason Merrill via Gcc-patches wrote:
>>> On 4/8/20 4:47 AM, Richard Biener wrote:
>>>> On Tue, Apr 7, 2020 at 5:01 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>
>>>>> Hi.
>>>>>
>>>>> The patch allows DCE to remove only replaceable operators new and delete.
>>>>> That's achieved by proper mark up of all these operators in C++ FE.
>>>>> The patch also brings all tests we've collected so far for the PR.
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>
>>>>> Ready to be installed?
>>>>
>>>> Grepping for uses of DECL_IS_OPERATOR_* reveals you miss comparing
>>>> the new flag in ipa-icf.c and cgraph node dumping in cgraph.c might want
>>>> to dump it as well.
>>>>
>>>> Otherwise it looks reasonable.
>>>>
>>>> So the mid-end parts are OK in case FE people are happy with this solution
>>>> for GCC 10.
>>>
>>> This seems fine for GCC 10, though I wonder about using an attribute for
>>> DECL_REPLACEABLE_OPERATOR rather than taking a bit in all FUNCTION_DECLs
>>> that will only ever be set on a small handful.
>>
>> If it is just for GCC 10 and we'll return the bit back to unused in GCC 11,
>> I'd think it is acceptable.
> 
> Agreed.
> 
> Jason
> 

All right, thanks for review. There's updated version of the patch that fullfils
Richi's comments.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
I'm going to install the patch.

Thanks,
Martin

[-- Attachment #2: 0001-Allow-new-delete-operator-deletion-only-for-replacea.patch --]
[-- Type: text/x-patch, Size: 17870 bytes --]

From 2f8ba3418f10b41bb839aadb292447bd757238d0 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 7 Apr 2020 16:23:27 +0200
Subject: [PATCH] Allow new/delete operator deletion only for replaceable.

gcc/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* gimple.c (gimple_call_operator_delete_p): Rename to...
	(gimple_call_replaceable_operator_delete_p): ... this.
	Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
	* gimple.h (gimple_call_operator_delete_p): Rename to ...
	(gimple_call_replaceable_operator_delete_p): ... this.
	* tree-core.h (tree_function_decl): Add replaceable_operator
	flag.
	* tree-ssa-dce.c (mark_all_reaching_defs_necessary_1):
	Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
	(propagate_necessity): Use gimple_call_replaceable_operator_delete_p.
	(eliminate_unnecessary_stmts): Likewise.
	* tree-streamer-in.c (unpack_ts_function_decl_value_fields):
	Pack DECL_IS_REPLACEABLE_OPERATOR.
	* tree-streamer-out.c (pack_ts_function_decl_value_fields):
	Unpack the field here.
	* tree.h (DECL_IS_REPLACEABLE_OPERATOR): New.
	(DECL_IS_REPLACEABLE_OPERATOR_NEW_P): New.
	(DECL_IS_REPLACEABLE_OPERATOR_DELETE_P): New.
	* cgraph.c (cgraph_node::dump): Dump if an operator is replaceable.
	* ipa-icf.c (sem_item::compare_referenced_symbol_properties): Compare
	replaceable operator flags.

gcc/cp/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* decl.c (duplicate_decls): Duplicate also DECL_IS_REPLACEABLE_OPERATOR.
	(cxx_init_decl_processing): Mark replaceable all implicitly defined
	operators.

gcc/lto/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* lto-common.c (compare_tree_sccs_1): Compare also
	DECL_IS_REPLACEABLE_OPERATOR.

gcc/testsuite/ChangeLog:

2020-04-07  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* g++.dg/pr94314-2.C: New test.
	* g++.dg/pr94314-3.C: New test.
	* g++.dg/pr94314.C: New test.
---
 gcc/cgraph.c                     |  7 +--
 gcc/cp/decl.c                    | 14 ++++++
 gcc/gimple.c                     |  6 +--
 gcc/gimple.h                     |  2 +-
 gcc/ipa-icf.c                    |  4 ++
 gcc/lto/lto-common.c             |  1 +
 gcc/testsuite/g++.dg/pr94314-2.C | 26 ++++++++++
 gcc/testsuite/g++.dg/pr94314-3.C | 55 +++++++++++++++++++++
 gcc/testsuite/g++.dg/pr94314.C   | 85 ++++++++++++++++++++++++++++++++
 gcc/tree-core.h                  |  3 +-
 gcc/tree-ssa-dce.c               |  8 +--
 gcc/tree-streamer-in.c           |  1 +
 gcc/tree-streamer-out.c          |  1 +
 gcc/tree.h                       | 10 +++-
 14 files changed, 210 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-2.C
 create mode 100644 gcc/testsuite/g++.dg/pr94314-3.C
 create mode 100644 gcc/testsuite/g++.dg/pr94314.C

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 6b780f80eb3..ecb234d032f 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -2157,10 +2157,11 @@ cgraph_node::dump (FILE *f)
   if (parallelized_function)
     fprintf (f, " parallelized_function");
   if (DECL_IS_OPERATOR_NEW_P (decl))
-    fprintf (f, " operator_new");
+    fprintf (f, " %soperator_new",
+	     DECL_IS_REPLACEABLE_OPERATOR (decl) ? "replaceable_" : "");
   if (DECL_IS_OPERATOR_DELETE_P (decl))
-    fprintf (f, " operator_delete");
-
+    fprintf (f, " %soperator_delete",
+	     DECL_IS_REPLACEABLE_OPERATOR (decl) ? "replaceable_" : "");
 
   fprintf (f, "\n");
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index a127734af69..a731702bdd2 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2368,6 +2368,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
 	    DECL_SET_IS_OPERATOR_NEW (newdecl, true);
 	  DECL_LOOPING_CONST_OR_PURE_P (newdecl)
 	    |= DECL_LOOPING_CONST_OR_PURE_P (olddecl);
+	  DECL_IS_REPLACEABLE_OPERATOR (newdecl)
+	    |= DECL_IS_REPLACEABLE_OPERATOR (olddecl);
 
 	  if (merge_attr)
 	    merge_attribute_bits (newdecl, olddecl);
@@ -4438,13 +4440,17 @@ cxx_init_decl_processing (void)
     tree opnew = push_cp_library_fn (NEW_EXPR, newtype, 0);
     DECL_IS_MALLOC (opnew) = 1;
     DECL_SET_IS_OPERATOR_NEW (opnew, true);
+    DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
     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;
     opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
     DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
     if (flag_sized_deallocation)
       {
 	/* Also push the sized deallocation variants:
@@ -4458,8 +4464,10 @@ cxx_init_decl_processing (void)
 	deltype = build_exception_variant (deltype, empty_except_spec);
 	opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
       }
 
     if (aligned_new_threshold)
@@ -4478,9 +4486,11 @@ cxx_init_decl_processing (void)
 	opnew = push_cp_library_fn (NEW_EXPR, newtype, 0);
 	DECL_IS_MALLOC (opnew) = 1;
 	DECL_SET_IS_OPERATOR_NEW (opnew, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
 	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;
 
 	/* operator delete (void *, align_val_t); */
 	deltype = build_function_type_list (void_type_node, ptr_type_node,
@@ -4489,8 +4499,10 @@ cxx_init_decl_processing (void)
 	deltype = build_exception_variant (deltype, empty_except_spec);
 	opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 
 	if (flag_sized_deallocation)
 	  {
@@ -4502,8 +4514,10 @@ cxx_init_decl_processing (void)
 	    deltype = build_exception_variant (deltype, empty_except_spec);
 	    opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
 	    DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	    opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
 	    DECL_SET_IS_OPERATOR_DELETE (opdel, true);
+	    DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
 	  }
       }
 
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 61a400b356f..10c562f4def 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2730,15 +2730,15 @@ gimple_builtin_call_types_compatible_p (const gimple *stmt, tree fndecl)
   return true;
 }
 
-/* Return true when STMT is operator delete call.  */
+/* Return true when STMT is operator a replaceable delete call.  */
 
 bool
-gimple_call_operator_delete_p (const gcall *stmt)
+gimple_call_replaceable_operator_delete_p (const gcall *stmt)
 {
   tree fndecl;
 
   if ((fndecl = gimple_call_fndecl (stmt)) != NULL_TREE)
-    return DECL_IS_OPERATOR_DELETE_P (fndecl);
+    return DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (fndecl);
   return false;
 }
 
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 305d98f5438..ca7fec6247e 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1615,7 +1615,7 @@ extern alias_set_type gimple_get_alias_set (tree);
 extern bool gimple_ior_addresses_taken (bitmap, gimple *);
 extern bool gimple_builtin_call_types_compatible_p (const gimple *, tree);
 extern combined_fn gimple_call_combined_fn (const gimple *);
-extern bool gimple_call_operator_delete_p (const gcall *);
+extern bool gimple_call_replaceable_operator_delete_p (const gcall *);
 extern bool gimple_call_builtin_p (const gimple *);
 extern bool gimple_call_builtin_p (const gimple *, enum built_in_class);
 extern bool gimple_call_builtin_p (const gimple *, enum built_in_function);
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 17a0ed9760b..069de9d82fb 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -347,6 +347,10 @@ sem_item::compare_referenced_symbol_properties (symtab_node *used_by,
       if (DECL_IS_OPERATOR_NEW_P (n1->decl)
 	  != DECL_IS_OPERATOR_NEW_P (n2->decl))
 	return return_false_with_msg ("operator new flags are different");
+
+      if (DECL_IS_REPLACEABLE_OPERATOR (n1->decl)
+	  != DECL_IS_REPLACEABLE_OPERATOR (n2->decl))
+	return return_false_with_msg ("replaceable operator flags are different");
     }
 
   /* Merging two definitions with a reference to equivalent vtables, but
diff --git a/gcc/lto/lto-common.c b/gcc/lto/lto-common.c
index c95a9b00d9e..e073abce2e7 100644
--- a/gcc/lto/lto-common.c
+++ b/gcc/lto/lto-common.c
@@ -1236,6 +1236,7 @@ compare_tree_sccs_1 (tree t1, tree t2, tree **map)
       compare_values (DECL_DISREGARD_INLINE_LIMITS);
       compare_values (DECL_PURE_P);
       compare_values (DECL_LOOPING_CONST_OR_PURE_P);
+      compare_values (DECL_IS_REPLACEABLE_OPERATOR);
       compare_values (DECL_FINAL_P);
       compare_values (DECL_CXX_CONSTRUCTOR_P);
       compare_values (DECL_CXX_DESTRUCTOR_P);
diff --git a/gcc/testsuite/g++.dg/pr94314-2.C b/gcc/testsuite/g++.dg/pr94314-2.C
new file mode 100644
index 00000000000..36b93ed6d4d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-2.C
@@ -0,0 +1,26 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+struct A
+{
+  __attribute__((always_inline)) A(int x)
+  {
+    if (x == 123)
+      throw x;
+  }
+};
+
+int
+main(int argc, char **argv)
+{
+  A *a = new A (argc);
+  delete a;
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 2 "cddce1"} } */
diff --git a/gcc/testsuite/g++.dg/pr94314-3.C b/gcc/testsuite/g++.dg/pr94314-3.C
new file mode 100644
index 00000000000..a5b10139290
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-3.C
@@ -0,0 +1,55 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 --param early-inlining-insns=100 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+volatile int idx;
+
+struct base
+{
+  __attribute__ ((malloc, noinline)) static void *
+  operator new (unsigned long sz)
+  {
+    return ::operator new (sz);
+  }
+
+  __attribute__ ((noinline)) static void operator delete (void *ptr)
+  {
+    int c = count[idx];
+    count[idx] = c - 1;
+    ::operator delete (ptr);
+  }
+  volatile static int count[2];
+};
+
+volatile int base::count[2] = {0, 0};
+
+struct B : base
+{
+  static void *operator new (unsigned long sz)
+  {
+    int c = count[idx];
+    count[idx] = c + 1;
+    return base::operator new (sz);
+  }
+};
+
+volatile int c = 1;
+
+int
+main ()
+{
+  for (int i; i < c; i++)
+    {
+      idx = 0;
+      delete new B;
+      if (B::count[0] != 0)
+	__builtin_abort ();
+    }
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/testsuite/g++.dg/pr94314.C b/gcc/testsuite/g++.dg/pr94314.C
new file mode 100644
index 00000000000..a06800d2219
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314.C
@@ -0,0 +1,85 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+struct A
+{
+  __attribute__((malloc,noinline))
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int A::count = 0;
+
+struct B
+{
+  __attribute__((malloc,noinline))
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  __attribute__((noinline))
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int B::count = 0;
+
+struct C
+{
+  static void* operator new(unsigned long sz)
+  {
+    ++count;
+    return ::operator new(sz);
+  }
+
+  static void operator delete(void* ptr)
+  {
+    --count;
+    ::operator delete(ptr);
+  }
+
+  static int count;
+};
+
+int C::count = 0;
+
+int main(){
+  delete new A;
+  if (A::count != 0)
+    __builtin_abort ();
+
+  delete new B;
+  if (B::count != 0)
+    __builtin_abort ();
+
+  delete new C;
+  if (C::count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 1 "cddce1"} } */
+/* { dg-final { scan-tree-dump-not "Deleting : B::operator delete" "cddce1"} } */
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index 765ea2a9542..d84fe959acc 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1896,8 +1896,9 @@ struct GTY(()) tree_function_decl {
   ENUM_BITFIELD(function_decl_type) decl_type: 2;
   unsigned has_debug_args_flag : 1;
   unsigned versioned_function : 1;
+  unsigned replaceable_operator : 1;
 
-  /* 12 bits left for future expansion.  */
+  /* 11 bits left for future expansion.  */
 };
 
 struct GTY(()) tree_translation_unit_decl {
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index e4077b58890..fd5f24c746c 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -614,7 +614,7 @@ mark_all_reaching_defs_necessary_1 (ao_ref *ref ATTRIBUTE_UNUSED,
 
       if (callee != NULL_TREE
 	  && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee)
-	      || DECL_IS_OPERATOR_DELETE_P (callee)))
+	      || DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (callee)))
 	return false;
     }
 
@@ -806,7 +806,7 @@ propagate_necessity (bool aggressive)
 	     processing the argument.  */
 	  bool is_delete_operator
 	    = (is_gimple_call (stmt)
-	       && gimple_call_operator_delete_p (as_a <gcall *> (stmt)));
+	       && gimple_call_replaceable_operator_delete_p (as_a <gcall *> (stmt)));
 	  if (is_delete_operator
 	      || gimple_call_builtin_p (stmt, BUILT_IN_FREE))
 	    {
@@ -896,7 +896,7 @@ propagate_necessity (bool aggressive)
 
 	      if (callee != NULL_TREE
 		  && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee)
-		      || DECL_IS_OPERATOR_DELETE_P (callee)))
+		      || DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (callee)))
 		continue;
 
 	      /* Calls implicitly load from memory, their arguments
@@ -1321,7 +1321,7 @@ eliminate_unnecessary_stmts (void)
 	  if (gimple_plf (stmt, STMT_NECESSARY)
 	      && (gimple_call_builtin_p (stmt, BUILT_IN_FREE)
 		  || (is_gimple_call (stmt)
-		      && gimple_call_operator_delete_p (as_a <gcall *> (stmt)))))
+		      && gimple_call_replaceable_operator_delete_p (as_a <gcall *> (stmt)))))
 	    {
 	      tree ptr = gimple_call_arg (stmt, 0);
 	      if (TREE_CODE (ptr) == SSA_NAME)
diff --git a/gcc/tree-streamer-in.c b/gcc/tree-streamer-in.c
index 4e035e9638f..0bfc272d076 100644
--- a/gcc/tree-streamer-in.c
+++ b/gcc/tree-streamer-in.c
@@ -343,6 +343,7 @@ unpack_ts_function_decl_value_fields (struct bitpack_d *bp, tree expr)
   DECL_DISREGARD_INLINE_LIMITS (expr) = (unsigned) bp_unpack_value (bp, 1);
   DECL_PURE_P (expr) = (unsigned) bp_unpack_value (bp, 1);
   DECL_LOOPING_CONST_OR_PURE_P (expr) = (unsigned) bp_unpack_value (bp, 1);
+  DECL_IS_REPLACEABLE_OPERATOR (expr) = (unsigned) bp_unpack_value (bp, 1);
   unsigned int fcode = 0;
   if (cl != NOT_BUILT_IN)
     {
diff --git a/gcc/tree-streamer-out.c b/gcc/tree-streamer-out.c
index 8e5e1355196..5bbcebba87e 100644
--- a/gcc/tree-streamer-out.c
+++ b/gcc/tree-streamer-out.c
@@ -305,6 +305,7 @@ pack_ts_function_decl_value_fields (struct bitpack_d *bp, tree expr)
   bp_pack_value (bp, DECL_DISREGARD_INLINE_LIMITS (expr), 1);
   bp_pack_value (bp, DECL_PURE_P (expr), 1);
   bp_pack_value (bp, DECL_LOOPING_CONST_OR_PURE_P (expr), 1);
+  bp_pack_value (bp, DECL_IS_REPLACEABLE_OPERATOR (expr), 1);
   if (DECL_BUILT_IN_CLASS (expr) != NOT_BUILT_IN)
     bp_pack_value (bp, DECL_UNCHECKED_FUNCTION_CODE (expr), 32);
 }
diff --git a/gcc/tree.h b/gcc/tree.h
index 66dfa876256..1c28785d411 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3037,6 +3037,11 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
     FUNCTION_DECL_DECL_TYPE (decl) = NONE;
 }
 
+/* Nonzero in a FUNCTION_DECL means this function is a replaceable
+   function (like replaceable operators new or delete).  */
+#define DECL_IS_REPLACEABLE_OPERATOR(NODE)\
+   (FUNCTION_DECL_CHECK (NODE)->function_decl.replaceable_operator)
+
 /* Nonzero in a FUNCTION_DECL means this function should be treated as
    C++ operator new, meaning that it returns a pointer for which we
    should not use type based aliasing.  */
@@ -3044,7 +3049,7 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
   (FUNCTION_DECL_CHECK (NODE)->function_decl.decl_type == OPERATOR_NEW)
 
 #define DECL_IS_REPLACEABLE_OPERATOR_NEW_P(NODE) \
-  (DECL_IS_OPERATOR_NEW_P (NODE) && DECL_IS_MALLOC (NODE))
+  (DECL_IS_OPERATOR_NEW_P (NODE) && DECL_IS_REPLACEABLE_OPERATOR (NODE))
 
 #define DECL_SET_IS_OPERATOR_NEW(NODE, VAL) \
   set_function_decl_type (FUNCTION_DECL_CHECK (NODE), OPERATOR_NEW, VAL)
@@ -3054,6 +3059,9 @@ set_function_decl_type (tree decl, function_decl_type t, bool set)
 #define DECL_IS_OPERATOR_DELETE_P(NODE) \
   (FUNCTION_DECL_CHECK (NODE)->function_decl.decl_type == OPERATOR_DELETE)
 
+#define DECL_IS_REPLACEABLE_OPERATOR_DELETE_P(NODE) \
+  (DECL_IS_OPERATOR_DELETE_P (NODE) && DECL_IS_REPLACEABLE_OPERATOR (NODE))
+
 #define DECL_SET_IS_OPERATOR_DELETE(NODE, VAL) \
   set_function_decl_type (FUNCTION_DECL_CHECK (NODE), OPERATOR_DELETE, VAL)
 
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jan Hubicka]

> From 2f8ba3418f10b41bb839aadb292447bd757238d0 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Tue, 7 Apr 2020 16:23:27 +0200
> Subject: [PATCH] Allow new/delete operator deletion only for replaceable.
> 
> gcc/ChangeLog:
> 
> 2020-04-07  Martin Liska  <mliska@suse.cz>
> 
> 	PR c++/94314
> 	* gimple.c (gimple_call_operator_delete_p): Rename to...
> 	(gimple_call_replaceable_operator_delete_p): ... this.
> 	Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
> 	* gimple.h (gimple_call_operator_delete_p): Rename to ...
> 	(gimple_call_replaceable_operator_delete_p): ... this.
> 	* tree-core.h (tree_function_decl): Add replaceable_operator
> 	flag.
> 	* tree-ssa-dce.c (mark_all_reaching_defs_necessary_1):
> 	Use DECL_IS_REPLACEABLE_OPERATOR_DELETE_P.
> 	(propagate_necessity): Use gimple_call_replaceable_operator_delete_p.
> 	(eliminate_unnecessary_stmts): Likewise.
> 	* tree-streamer-in.c (unpack_ts_function_decl_value_fields):
> 	Pack DECL_IS_REPLACEABLE_OPERATOR.
> 	* tree-streamer-out.c (pack_ts_function_decl_value_fields):
> 	Unpack the field here.
> 	* tree.h (DECL_IS_REPLACEABLE_OPERATOR): New.
> 	(DECL_IS_REPLACEABLE_OPERATOR_NEW_P): New.
> 	(DECL_IS_REPLACEABLE_OPERATOR_DELETE_P): New.
> 	* cgraph.c (cgraph_node::dump): Dump if an operator is replaceable.
> 	* ipa-icf.c (sem_item::compare_referenced_symbol_properties): Compare
> 	replaceable operator flags.
> 
> gcc/cp/ChangeLog:
> 
> 2020-04-07  Martin Liska  <mliska@suse.cz>
> 
> 	PR c++/94314
> 	* decl.c (duplicate_decls): Duplicate also DECL_IS_REPLACEABLE_OPERATOR.
> 	(cxx_init_decl_processing): Mark replaceable all implicitly defined
> 	operators.
> 
> gcc/lto/ChangeLog:
> 
> 2020-04-07  Martin Liska  <mliska@suse.cz>
> 
> 	PR c++/94314
> 	* lto-common.c (compare_tree_sccs_1): Compare also
> 	DECL_IS_REPLACEABLE_OPERATOR.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-04-07  Martin Liska  <mliska@suse.cz>
> 
> 	PR c++/94314
> 	* g++.dg/pr94314-2.C: New test.
> 	* g++.dg/pr94314-3.C: New test.
> 	* g++.dg/pr94314.C: New test.
> @@ -2368,6 +2368,8 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
>  	    DECL_SET_IS_OPERATOR_NEW (newdecl, true);
>  	  DECL_LOOPING_CONST_OR_PURE_P (newdecl)
>  	    |= DECL_LOOPING_CONST_OR_PURE_P (olddecl);
> +	  DECL_IS_REPLACEABLE_OPERATOR (newdecl)
> +	    |= DECL_IS_REPLACEABLE_OPERATOR (olddecl);
>  
>  	  if (merge_attr)
>  	    merge_attribute_bits (newdecl, olddecl);

I think you will also need to take care of decl copying tht happens in
the midle-end.  If we do ipa-cp clone of the replaceable operator I
guess we want to copy the flag since it is still new/delete
(removeing it in dce has still the same semantics)

However if we split the function, I think we want to remove the flag
from foo.part clone, or one would be able to again produce the examples
with broken reference counting by making ipa-split to separate the
refernece count and tail of the function.

I am not quite sure about the offloading done by openMP - I think that
one produces new decls.

Honza

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jakub Jelinek]

On Wed, Apr 08, 2020 at 05:46:52PM +0200, Jan Hubicka wrote:
> I am not quite sure about the offloading done by openMP - I think that
> one produces new decls.

Yes, it does.  It copies some FUNCTION_DECL flags over, but only selected
ones (and all attributes but removes a few afterwards).

	Jakub

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

Hi.

We've got one another sneaky test-case (thank you Marc ;) ):

$ cat pr94314-array.C
#include <stdio.h>
#include <new>

int count = 0;

__attribute__((malloc, noinline)) void* operator new[](unsigned long sz) {
   ++count;
   return ::operator new(sz);
}

void operator delete[](void* ptr) noexcept {
   --count;
   ::operator delete(ptr);
}

void operator delete[](void* ptr, std::size_t sz) noexcept {
   --count;
   ::operator delete(ptr, sz);
}

int main() {
   delete[] new int[1];
   if (count != 0)
     __builtin_abort ();
}

I bet we need to include the Honza's fix for inline stacks.
Or it the test-case invalid?

Thanks,
Martin

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Richard Biener]

On Thu, Apr 9, 2020 at 7:06 AM Martin Liška <mliska@suse.cz> wrote:
>
> Hi.
>
> We've got one another sneaky test-case (thank you Marc ;) ):
>
> $ cat pr94314-array.C
> #include <stdio.h>
> #include <new>
>
> int count = 0;
>
> __attribute__((malloc, noinline)) void* operator new[](unsigned long sz) {
>    ++count;
>    return ::operator new(sz);
> }
>
> void operator delete[](void* ptr) noexcept {
>    --count;
>    ::operator delete(ptr);
> }
>
> void operator delete[](void* ptr, std::size_t sz) noexcept {
>    --count;
>    ::operator delete(ptr, sz);
> }
>
> int main() {
>    delete[] new int[1];
>    if (count != 0)
>      __builtin_abort ();
> }
>
> I bet we need to include the Honza's fix for inline stacks.
> Or it the test-case invalid?

I don't see how inline stacking helps here when you consider

void *foo(unsigned long sz) { return ::operator new(sz); }
void operator delete[](void* ptr) noexcept {
    --count;
    ::operator delete(ptr);
}

thus regular functions inlining where definitely the inline
stack depth does not need to match.

I guess the testcase asks for us to match the exact
operator form (in the testcase we match ::delete and ::new[]),
for example by instead of looking at the decl flags
simply match the assembler names (the mangled names)
of the operator?

Richard.

> Thanks,
> Martin

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/9/20 8:45 AM, Richard Biener wrote:
> On Thu, Apr 9, 2020 at 7:06 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> Hi.
>>
>> We've got one another sneaky test-case (thank you Marc ;) ):
>>
>> $ cat pr94314-array.C
>> #include <stdio.h>
>> #include <new>
>>
>> int count = 0;
>>
>> __attribute__((malloc, noinline)) void* operator new[](unsigned long sz) {
>>     ++count;
>>     return ::operator new(sz);
>> }
>>
>> void operator delete[](void* ptr) noexcept {
>>     --count;
>>     ::operator delete(ptr);
>> }
>>
>> void operator delete[](void* ptr, std::size_t sz) noexcept {
>>     --count;
>>     ::operator delete(ptr, sz);
>> }
>>
>> int main() {
>>     delete[] new int[1];
>>     if (count != 0)
>>       __builtin_abort ();
>> }
>>
>> I bet we need to include the Honza's fix for inline stacks.
>> Or it the test-case invalid?
> 
> I don't see how inline stacking helps here when you consider
> 
> void *foo(unsigned long sz) { return ::operator new(sz); }
> void operator delete[](void* ptr) noexcept {
>      --count;
>      ::operator delete(ptr);
> }
> 
> thus regular functions inlining where definitely the inline
> stack depth does not need to match.

I was considering quite strict rules:
- inline stack can contain only up to 1 replaceable operator new (or delete)
- no non-replaceable operators are allowed
- number of repl. operator much match.

> 
> I guess the testcase asks for us to match the exact
> operator form (in the testcase we match ::delete and ::new[]),
> for example by instead of looking at the decl flags
> simply match the assembler names (the mangled names)
> of the operator?

What do you mean by 'decl flags'. We can't compare ASM names as one is ctor
and the second one is dtor. It's about argument types that much match, right?

Thanks,
Martin

> 
> Richard.
> 
>> Thanks,
>> Martin


[-- Attachment #2: 0001-Compare-inline-stacks-for-new-delete-repl.-operators.patch --]
[-- Type: text/x-patch, Size: 6250 bytes --]

From 16c77b6b65e31be657f91dcf0308f1638a784d7a Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 9 Apr 2020 07:36:56 +0200
Subject: [PATCH] Compare inline stacks for new/delete repl. operators.

gcc/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* tree-ssa-dce.c (compare_new_delete_inline_contexts):
	(propagate_necessity):
	* cgraphclones.c (set_new_clone_decl_and_node_flags):
	Drop DECL_IS_REPLACEABLE_OPERATOR to false.

gcc/testsuite/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* g++.dg/pr94314-4.C: New test.
	* g++.dg/pr94314.C: Do not expect a dtor deletion.
---
 gcc/cgraphclones.c               |  2 +
 gcc/testsuite/g++.dg/pr94314-4.C | 33 ++++++++++++++
 gcc/testsuite/g++.dg/pr94314.C   |  2 +-
 gcc/tree-ssa-dce.c               | 76 +++++++++++++++++++++++++++-----
 4 files changed, 102 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-4.C

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c73b8f810f0..8f541a28b6e 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -165,6 +165,7 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
   DECL_STATIC_DESTRUCTOR (new_node->decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_node->decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_node->decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_node->decl) = 0;
 
   new_node->externally_visible = 0;
   new_node->local = 1;
@@ -1030,6 +1031,7 @@ cgraph_node::create_version_clone_with_body
   DECL_STATIC_DESTRUCTOR (new_decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_decl) = 0;
 
   /* Create the new version's call-graph node.
      and update the edges of the new node. */
diff --git a/gcc/testsuite/g++.dg/pr94314-4.C b/gcc/testsuite/g++.dg/pr94314-4.C
new file mode 100644
index 00000000000..afa2a443dc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-4.C
@@ -0,0 +1,33 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+int count = 0;
+
+__attribute__((malloc, noinline)) void* operator new[](__SIZE_TYPE__ sz) {
+  ++count;
+  return ::operator new(sz);
+}
+
+void operator delete[](void* ptr) noexcept {
+  --count;
+  ::operator delete(ptr);
+}
+
+void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
+  --count;
+  ::operator delete(ptr, sz);
+}
+
+int main() {
+  delete[] new int[1];
+  if (count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/testsuite/g++.dg/pr94314.C b/gcc/testsuite/g++.dg/pr94314.C
index 86e651d10ba..da293a7dd9b 100644
--- a/gcc/testsuite/g++.dg/pr94314.C
+++ b/gcc/testsuite/g++.dg/pr94314.C
@@ -81,5 +81,5 @@ int main(){
   return 0;
 }
 
-/* { dg-final { scan-tree-dump-times "Deleting : operator delete" 1 "cddce1"} } */
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
 /* { dg-final { scan-tree-dump-not "Deleting : B::operator delete" "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index fd5f24c746c..e1085d6b049 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,53 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Compare new/delete inline contexts and return false if
+   either number of replaceable operators is different or we meet
+  a non-replaceable operator.  */
+
+static bool
+compare_new_delete_inline_contexts (gimple *new_call, gimple *delete_call)
+{
+  unsigned int replaceable_new_ops = 0;
+  unsigned int replaceable_delete_ops = 0;
+
+  for (tree block = gimple_block (new_call);
+       block && TREE_CODE (block) == BLOCK; block = BLOCK_SUPERCONTEXT (block))
+    {
+      tree fn = block_ultimate_origin (block);
+      if (fn != NULL && TREE_CODE (fn) == FUNCTION_DECL)
+	{
+	  if (DECL_IS_OPERATOR_NEW_P (fn))
+	    {
+	      if (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fn))
+		++replaceable_new_ops;
+	      else
+		return false;
+	    }
+	}
+    }
+
+  for (tree block = gimple_block (delete_call);
+       block && TREE_CODE (block) == BLOCK; block = BLOCK_SUPERCONTEXT (block))
+    {
+      tree fn = block_ultimate_origin (block);
+      if (fn != NULL && TREE_CODE (fn) == FUNCTION_DECL)
+	{
+	  if (DECL_IS_OPERATOR_DELETE_P (fn))
+	    {
+	      if (DECL_IS_REPLACEABLE_OPERATOR_DELETE_P (fn))
+		++replaceable_delete_ops;
+	      else
+		return false;
+	    }
+	}
+    }
+
+  return (replaceable_new_ops <= 1
+	  && replaceable_delete_ops <= 1
+	  && replaceable_new_ops == replaceable_delete_ops);
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +871,25 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      /* Verify that new and delete operators have the same
+			 context.  */
+		      if (!compare_new_delete_inline_contexts (def_stmt, stmt))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Richard Biener]

On Thu, Apr 9, 2020 at 9:00 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 4/9/20 8:45 AM, Richard Biener wrote:
> > On Thu, Apr 9, 2020 at 7:06 AM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> Hi.
> >>
> >> We've got one another sneaky test-case (thank you Marc ;) ):
> >>
> >> $ cat pr94314-array.C
> >> #include <stdio.h>
> >> #include <new>
> >>
> >> int count = 0;
> >>
> >> __attribute__((malloc, noinline)) void* operator new[](unsigned long sz) {
> >>     ++count;
> >>     return ::operator new(sz);
> >> }
> >>
> >> void operator delete[](void* ptr) noexcept {
> >>     --count;
> >>     ::operator delete(ptr);
> >> }
> >>
> >> void operator delete[](void* ptr, std::size_t sz) noexcept {
> >>     --count;
> >>     ::operator delete(ptr, sz);
> >> }
> >>
> >> int main() {
> >>     delete[] new int[1];
> >>     if (count != 0)
> >>       __builtin_abort ();
> >> }
> >>
> >> I bet we need to include the Honza's fix for inline stacks.
> >> Or it the test-case invalid?
> >
> > I don't see how inline stacking helps here when you consider
> >
> > void *foo(unsigned long sz) { return ::operator new(sz); }
> > void operator delete[](void* ptr) noexcept {
> >      --count;
> >      ::operator delete(ptr);
> > }
> >
> > thus regular functions inlining where definitely the inline
> > stack depth does not need to match.
>
> I was considering quite strict rules:
> - inline stack can contain only up to 1 replaceable operator new (or delete)
> - no non-replaceable operators are allowed
> - number of repl. operator much match.
>
> >
> > I guess the testcase asks for us to match the exact
> > operator form (in the testcase we match ::delete and ::new[]),
> > for example by instead of looking at the decl flags
> > simply match the assembler names (the mangled names)
> > of the operator?
>
> What do you mean by 'decl flags'. We can't compare ASM names as one is ctor
> and the second one is dtor. It's about argument types that much match, right?

Sure, we have to make a translation from delete to new ODR name and compare
those.  I thought of simply having an array of predefined pairs like

{ { "_Znam", "_ZdaPvm" }, ... }

if programmatically translating the ODR name of the delete to the corresponding
new one is too difficult.

Richard.

> Thanks,
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
>

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jakub Jelinek]

On Thu, Apr 09, 2020 at 08:59:59AM +0200, Martin Liška wrote:
> What do you mean by 'decl flags'. We can't compare ASM names as one is ctor
> and the second one is dtor. It's about argument types that much match, right?

You can't disginguish the [] vs. non-[] from arguments, plus there are some
changes in the arguments in all cases anyway (e.g. first argument of
operator delete always being the pointer, while first argument of operator
new being size_t, but some operator delete forms also having size_t after
it).
See http://eel.is/c++draft/new.delete 
[[nodiscard]] void* operator new(std::size_t size);
[[nodiscard]] void* operator new(std::size_t size, std::align_val_t alignment);
[[nodiscard]] void* operator new(std::size_t size, const std::nothrow_t&) noexcept;
[[nodiscard]] void* operator new(std::size_t size, std::align_val_t alignment, const std::nothrow_t&) noexcept;
void operator delete(void* ptr) noexcept;
void operator delete(void* ptr, std::size_t size) noexcept;
void operator delete(void* ptr, std::align_val_t alignment) noexcept;
void operator delete(void* ptr, std::size_t size, std::align_val_t alignment) noexcept;
void operator delete(void* ptr, const std::nothrow_t&) noexcept;
void operator delete(void* ptr, std::align_val_t alignment, const std::nothrow_t&) noexcept;
[[nodiscard]] void* operator new[](std::size_t size);
[[nodiscard]] void* operator new[](std::size_t size, std::align_val_t alignment);
[[nodiscard]] void* operator new[](std::size_t size, const std::nothrow_t&) noexcept;
[[nodiscard]] void* operator new[](std::size_t size, std::align_val_t alignment, const std::nothrow_t&) noexcept;
void operator delete[](void* ptr) noexcept;
void operator delete[](void* ptr, std::size_t size) noexcept;
void operator delete[](void* ptr, std::align_val_t alignment) noexcept;
void operator delete[](void* ptr, std::size_t size, std::align_val_t alignment) noexcept;
void operator delete[](void* ptr, const std::nothrow_t&) noexcept;
void operator delete[](void* ptr, std::align_val_t alignment, const std::nothrow_t&) noexcept;

I believe one can't allocate with operator new and free with operator delete[]
or allocate with operator new[] and free with operator delete, but am not
sure if there aren't other requirements?
E.g. "If the alignment parameter is not present, ptr was returned by an allocation function
without an alignment parameter.  If present, the alignment argument is equal to the
alignment argument passed to the allocation function that returned ptr."
suggests one can't mix the std::align_val_t vs. non-std::align_val_t cases,
dunno about the throwing vs. non-throwing or sizd ones.

	Jakub

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Marc Glisse]

On Thu, 9 Apr 2020, Richard Biener wrote:

> On Thu, Apr 9, 2020 at 7:06 AM Martin Liška <mliska@suse.cz> wrote:
>>
>> Hi.
>>
>> We've got one another sneaky test-case (thank you Marc ;) ):
>>
>> $ cat pr94314-array.C
>> #include <stdio.h>
>> #include <new>
>>
>> int count = 0;
>>
>> __attribute__((malloc, noinline)) void* operator new[](unsigned long sz) {
>>    ++count;
>>    return ::operator new(sz);
>> }
>>
>> void operator delete[](void* ptr) noexcept {
>>    --count;
>>    ::operator delete(ptr);
>> }
>>
>> void operator delete[](void* ptr, std::size_t sz) noexcept {
>>    --count;
>>    ::operator delete(ptr, sz);
>> }
>>
>> int main() {
>>    delete[] new int[1];
>>    if (count != 0)
>>      __builtin_abort ();
>> }
>>
>> I bet we need to include the Honza's fix for inline stacks.
>> Or it the test-case invalid?
>
> I don't see how inline stacking helps here when you consider
>
> void *foo(unsigned long sz) { return ::operator new(sz); }
> void operator delete[](void* ptr) noexcept {
>    --count;
>    ::operator delete(ptr);
> }
>
> thus regular functions inlining where definitely the inline
> stack depth does not need to match.
>
> I guess the testcase asks for us to match the exact
> operator form (in the testcase we match ::delete and ::new[]),
> for example by instead of looking at the decl flags
> simply match the assembler names (the mangled names)
> of the operator?

A hard-coded list can make sense (although if we use asm names, I guess we 
have to take care of platforms that prefix all symbols with _ for 
instance). Note that the matching is not 1-to-1. Array vs non-array and 
aligned vs non-aligned seem important, but sized and unsized delete can 
both match the same new, IIUC. Not sure about the nothrow versions...

-- 
Marc Glisse

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jonathan Wakely]

On Thu, 9 Apr 2020 at 09:05, Marc Glisse wrote:
> Note that the matching is not 1-to-1. Array vs non-array and
> aligned vs non-aligned seem important, but sized and unsized delete can
> both match the same new, IIUC.

Right.

> Not sure about the nothrow versions...

This is valid, and mixes the nothrow new with non-nothrow delete:

delete new (std::nothrow) int;

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jason Merrill]

On 4/9/20 1:05 AM, Martin Liška wrote:
> Hi.
> 
> We've got one another sneaky test-case (thank you Marc ;) ):
> 
> $ cat pr94314-array.C
> #include <stdio.h>
> #include <new>
> 
> int count = 0;
> 
> __attribute__((malloc, noinline)) void* operator new[](unsigned long sz) {
>    ++count;
>    return ::operator new(sz);
> }
> 
> void operator delete[](void* ptr) noexcept {
>    --count;
>    ::operator delete(ptr);
> }
> 
> void operator delete[](void* ptr, std::size_t sz) noexcept {
>    --count;
>    ::operator delete(ptr, sz);
> }
> 
> int main() {
>    delete[] new int[1];
>    if (count != 0)
>      __builtin_abort ();
> }
> 
> I bet we need to include the Honza's fix for inline stacks.
> Or it the test-case invalid?

I suppose that these inlining issues are a good reason why the standard 
talks about new-expressions and delete-expressions rather than calls.

Jason

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/9/20 10:13 AM, Jonathan Wakely wrote:
> On Thu, 9 Apr 2020 at 09:05, Marc Glisse wrote:
>> Note that the matching is not 1-to-1. Array vs non-array and
>> aligned vs non-aligned seem important, but sized and unsized delete can
>> both match the same new, IIUC.
> 
> Right.
> 
>> Not sure about the nothrow versions...
> 
> This is valid, and mixes the nothrow new with non-nothrow delete:
> 
> delete new (std::nothrow) int;
> 

All right, there's a patch candidate that comes up with the list of possible pairs.
For better readability, I present demangled names:

$ cat /tmp/pairs.txt | c++filt
	  "operator new(unsigned long):operator delete(void*)" ,
	  "operator new(unsigned long):operator delete(void*, unsigned long)" ,
	  "operator new(unsigned long):operator delete(void*, std::nothrow_t const&)" ,
	  "operator new(unsigned long, std::nothrow_t const&):operator delete(void*)" ,
	  "operator new(unsigned long, std::nothrow_t const&):operator delete(void*, unsigned long)" ,
	  "operator new(unsigned long, std::nothrow_t const&):operator delete(void*, std::nothrow_t const&)" ,
	  /* non-[] operators with alignment.  */
	  "operator new(unsigned long, std::align_val_t):operator delete(void*, unsigned long, std::align_val_t)" ,
	  "operator new(unsigned long, std::align_val_t):operator delete(void*, std::align_val_t)" ,
	  "operator new(unsigned long, std::align_val_t):operator delete(void*, std::align_val_t, std::nothrow_t const&)" ,
	  "operator new(unsigned long, std::align_val_t, std::nothrow_t const&):operator delete(void*, unsigned long, std::align_val_t)" ,
	  "operator new(unsigned long, std::align_val_t, std::nothrow_t const&):operator delete(void*, std::align_val_t)" ,
	  "operator new(unsigned long, std::align_val_t, std::nothrow_t const&):operator delete(void*, std::align_val_t, std::nothrow_t const&)" ,
	  /* [] operators.  */
	   "operator new[](unsigned long):operator delete[](void*)" ,
	   "operator new[](unsigned long):operator delete[](void*, unsigned long)" ,
	   "operator new[](unsigned long):operator delete[](void*, std::nothrow_t const&)" ,
	   "operator new[](unsigned long, std::nothrow_t const&):operator delete[](void*)" ,
	   "operator new[](unsigned long, std::nothrow_t const&):operator delete[](void*, unsigned long)" ,
	   "operator new[](unsigned long, std::nothrow_t const&):operator delete[](void*, std::nothrow_t const&)" ,
	  /* [] operators with alignment.  */
	   "operator new[](unsigned long, std::align_val_t):operator delete[](void*, unsigned long, std::align_val_t)" ,
	   "operator new[](unsigned long, std::align_val_t):operator delete[](void*, std::align_val_t)" ,
	   "operator new[](unsigned long, std::align_val_t):operator delete[](void*, std::align_val_t, std::nothrow_t const&)" ,
	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t const&):operator delete[](void*, unsigned long, std::align_val_t)",
	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t const&):operator delete[](void*, std::align_val_t)",
	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t const&):operator delete[](void*, std::align_val_t, std::nothrow_t const&)",

Marc pointed out that some targets do not use the leading '_' (or use 2 dashes?) for mangled named?
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Thanks,
Martin

[-- Attachment #2: 0001-List-valid-pairs-for-new-and-delete-operators.patch --]
[-- Type: text/x-patch, Size: 6895 bytes --]

From aa7102bcd698b6f132e0b9ffecd847d68083039b Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 9 Apr 2020 15:50:58 +0200
Subject: [PATCH] List valid pairs for new and delete operators.

gcc/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* cgraphclones.c (set_new_clone_decl_and_node_flags): Drop
	DECL_IS_REPLACEABLE_OPERATOR during cloning.
	* tree-ssa-dce.c (valid_new_delete_pair_p): New function.
	(propagate_necessity): Check operator names.
	(perform_tree_ssa_dce): Delete valid_pairs.

gcc/testsuite/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* g++.dg/pr94314-4.C: New test.
---
 gcc/cgraphclones.c               |  2 +
 gcc/testsuite/g++.dg/pr94314-4.C | 33 ++++++++++++
 gcc/tree-ssa-dce.c               | 93 ++++++++++++++++++++++++++++----
 3 files changed, 118 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-4.C

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c73b8f810f0..8f541a28b6e 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -165,6 +165,7 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
   DECL_STATIC_DESTRUCTOR (new_node->decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_node->decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_node->decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_node->decl) = 0;
 
   new_node->externally_visible = 0;
   new_node->local = 1;
@@ -1030,6 +1031,7 @@ cgraph_node::create_version_clone_with_body
   DECL_STATIC_DESTRUCTOR (new_decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_decl) = 0;
 
   /* Create the new version's call-graph node.
      and update the edges of the new node. */
diff --git a/gcc/testsuite/g++.dg/pr94314-4.C b/gcc/testsuite/g++.dg/pr94314-4.C
new file mode 100644
index 00000000000..afa2a443dc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-4.C
@@ -0,0 +1,33 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+int count = 0;
+
+__attribute__((malloc, noinline)) void* operator new[](__SIZE_TYPE__ sz) {
+  ++count;
+  return ::operator new(sz);
+}
+
+void operator delete[](void* ptr) noexcept {
+  --count;
+  ::operator delete(ptr);
+}
+
+void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
+  --count;
+  ::operator delete(ptr, sz);
+}
+
+int main() {
+  delete[] new int[1];
+  if (count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index fd5f24c746c..e1074a803f8 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,70 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Valid pairs of new and delete operators for DCE.  */
+static hash_set<nofree_string_hash> *valid_pairs = NULL;
+
+/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
+   and delete  operators.  */
+
+static bool
+valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
+{
+  const char *new_name
+    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call)));
+  const char *delete_name
+    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call)));
+  char *needle = concat (new_name, ":", delete_name, NULL);
+
+  if (valid_pairs == NULL)
+    {
+      valid_pairs = new hash_set<nofree_string_hash> ();
+      /* Invalid pairs:
+	 non-[] and []
+	 aligned and non-aligned
+      */
+
+      const char *pairs[] = {
+	  /* non-[] operators.  */
+	  "_Znwm:_ZdlPv" ,
+	  "_Znwm:_ZdlPvm" ,
+	  "_Znwm:_ZdlPvRKSt9nothrow_t" ,
+	  "_ZnwmRKSt9nothrow_t:_ZdlPv" ,
+	  "_ZnwmRKSt9nothrow_t:_ZdlPvm" ,
+	  "_ZnwmRKSt9nothrow_t:_ZdlPvRKSt9nothrow_t" ,
+	  /* non-[] operators with alignment.  */
+	  "_ZnwmSt11align_val_t:_ZdlPvmSt11align_val_t" ,
+	  "_ZnwmSt11align_val_t:_ZdlPvSt11align_val_t" ,
+	  "_ZnwmSt11align_val_t:_ZdlPvSt11align_val_tRKSt9nothrow_t" ,
+	  "_ZnwmSt11align_val_tRKSt9nothrow_t:_ZdlPvmSt11align_val_t" ,
+	  "_ZnwmSt11align_val_tRKSt9nothrow_t:_ZdlPvSt11align_val_t" ,
+	  "_ZnwmSt11align_val_tRKSt9nothrow_t:_ZdlPvSt11align_val_tRKSt9nothrow_t" ,
+	  /* [] operators.  */
+	   "_Znam:_ZdaPv" ,
+	   "_Znam:_ZdaPvm" ,
+	   "_Znam:_ZdaPvRKSt9nothrow_t" ,
+	   "_ZnamRKSt9nothrow_t:_ZdaPv" ,
+	   "_ZnamRKSt9nothrow_t:_ZdaPvm" ,
+	   "_ZnamRKSt9nothrow_t:_ZdaPvRKSt9nothrow_t" ,
+	  /* [] operators with alignment.  */
+	   "_ZnamSt11align_val_t:_ZdaPvmSt11align_val_t" ,
+	   "_ZnamSt11align_val_t:_ZdaPvSt11align_val_t" ,
+	   "_ZnamSt11align_val_t:_ZdaPvSt11align_val_tRKSt9nothrow_t" ,
+	   "_ZnamSt11align_val_tRKSt9nothrow_t:_ZdaPvmSt11align_val_t",
+	   "_ZnamSt11align_val_tRKSt9nothrow_t:_ZdaPvSt11align_val_t",
+	   "_ZnamSt11align_val_tRKSt9nothrow_t:_ZdaPvSt11align_val_tRKSt9nothrow_t",
+	   NULL
+      };
+
+      for (unsigned i = 0; pairs[i] != NULL; i++)
+	valid_pairs->add (pairs[i]);
+    }
+
+  bool r = valid_pairs->contains (needle);
+  free (needle);
+  return r;
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +888,23 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      if (!valid_new_delete_pair_p (def_stmt, stmt))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
@@ -1662,6 +1733,8 @@ perform_tree_ssa_dce (bool aggressive)
   visited = BITMAP_ALLOC (NULL);
   propagate_necessity (aggressive);
   BITMAP_FREE (visited);
+  delete valid_pairs;
+  valid_pairs = NULL;
 
   something_changed |= eliminate_unnecessary_stmts ();
   something_changed |= cfg_altered;
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jonathan Wakely]

On Fri, 10 Apr 2020 at 09:08, Martin Liška <mliska@suse.cz> wrote:
> Marc pointed out that some targets do not use the leading '_' (or use 2 dashes?) for mangled named?

Double underscore at the start. I think darwin uses that.

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/10/20 10:18 AM, Jonathan Wakely wrote:
> On Fri, 10 Apr 2020 at 09:08, Martin Liška <mliska@suse.cz> wrote:
>> Marc pointed out that some targets do not use the leading '_' (or use 2 dashes?) for mangled named?
> 
> Double underscore at the start. I think darwin uses that.
> 

Ah yeah, not dashes, but underscores ;)
It's fixed in attached patch that I've been testing right now.

Martin

[-- Attachment #2: 0001-List-valid-pairs-for-new-and-delete-operators.patch --]
[-- Type: text/x-patch, Size: 7041 bytes --]

From 4043b187ee8e6d7d0c05a6463e38ca54ba4acf75 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 9 Apr 2020 15:50:58 +0200
Subject: [PATCH] List valid pairs for new and delete operators.

gcc/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* cgraphclones.c (set_new_clone_decl_and_node_flags): Drop
	DECL_IS_REPLACEABLE_OPERATOR during cloning.
	* tree-ssa-dce.c (valid_new_delete_pair_p): New function.
	(propagate_necessity): Check operator names.
	(perform_tree_ssa_dce): Delete valid_pairs.

gcc/testsuite/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>

	PR c++/94314
	* g++.dg/pr94314-4.C: New test.
---
 gcc/cgraphclones.c               |  2 +
 gcc/testsuite/g++.dg/pr94314-4.C | 33 +++++++++++
 gcc/tree-ssa-dce.c               | 99 ++++++++++++++++++++++++++++----
 3 files changed, 124 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-4.C

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c73b8f810f0..8f541a28b6e 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -165,6 +165,7 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
   DECL_STATIC_DESTRUCTOR (new_node->decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_node->decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_node->decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_node->decl) = 0;
 
   new_node->externally_visible = 0;
   new_node->local = 1;
@@ -1030,6 +1031,7 @@ cgraph_node::create_version_clone_with_body
   DECL_STATIC_DESTRUCTOR (new_decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_decl) = 0;
 
   /* Create the new version's call-graph node.
      and update the edges of the new node. */
diff --git a/gcc/testsuite/g++.dg/pr94314-4.C b/gcc/testsuite/g++.dg/pr94314-4.C
new file mode 100644
index 00000000000..afa2a443dc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-4.C
@@ -0,0 +1,33 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+int count = 0;
+
+__attribute__((malloc, noinline)) void* operator new[](__SIZE_TYPE__ sz) {
+  ++count;
+  return ::operator new(sz);
+}
+
+void operator delete[](void* ptr) noexcept {
+  --count;
+  ::operator delete(ptr);
+}
+
+void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
+  --count;
+  ::operator delete(ptr, sz);
+}
+
+int main() {
+  delete[] new int[1];
+  if (count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index fd5f24c746c..f66069e410d 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,76 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Valid pairs of new and delete operators for DCE.  */
+static hash_set<nofree_string_hash> *valid_pairs = NULL;
+
+/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
+   and delete  operators.  */
+
+static bool
+valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
+{
+  const char *new_name
+    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call)));
+  const char *delete_name
+    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call)));
+
+  if (new_name[0] == '_' and new_name[1] == '_')
+    ++new_name;
+  if (delete_name[0] == '_' and delete_name[1] == '_')
+    ++delete_name;
+
+  char *needle = concat (new_name, ":", delete_name, NULL);
+
+  if (valid_pairs == NULL)
+    {
+      valid_pairs = new hash_set<nofree_string_hash> ();
+      /* Invalid pairs:
+	 non-[] and []
+	 aligned and non-aligned
+      */
+
+      const char *pairs[] = {
+	  /* non-[] operators.  */
+	  "_Znwm:_ZdlPv" ,
+	  "_Znwm:_ZdlPvm" ,
+	  "_Znwm:_ZdlPvRKSt9nothrow_t" ,
+	  "_ZnwmRKSt9nothrow_t:_ZdlPv" ,
+	  "_ZnwmRKSt9nothrow_t:_ZdlPvm" ,
+	  "_ZnwmRKSt9nothrow_t:_ZdlPvRKSt9nothrow_t" ,
+	  /* non-[] operators with alignment.  */
+	  "_ZnwmSt11align_val_t:_ZdlPvmSt11align_val_t" ,
+	  "_ZnwmSt11align_val_t:_ZdlPvSt11align_val_t" ,
+	  "_ZnwmSt11align_val_t:_ZdlPvSt11align_val_tRKSt9nothrow_t" ,
+	  "_ZnwmSt11align_val_tRKSt9nothrow_t:_ZdlPvmSt11align_val_t" ,
+	  "_ZnwmSt11align_val_tRKSt9nothrow_t:_ZdlPvSt11align_val_t" ,
+	  "_ZnwmSt11align_val_tRKSt9nothrow_t:_ZdlPvSt11align_val_tRKSt9nothrow_t" ,
+	  /* [] operators.  */
+	   "_Znam:_ZdaPv" ,
+	   "_Znam:_ZdaPvm" ,
+	   "_Znam:_ZdaPvRKSt9nothrow_t" ,
+	   "_ZnamRKSt9nothrow_t:_ZdaPv" ,
+	   "_ZnamRKSt9nothrow_t:_ZdaPvm" ,
+	   "_ZnamRKSt9nothrow_t:_ZdaPvRKSt9nothrow_t" ,
+	  /* [] operators with alignment.  */
+	   "_ZnamSt11align_val_t:_ZdaPvmSt11align_val_t" ,
+	   "_ZnamSt11align_val_t:_ZdaPvSt11align_val_t" ,
+	   "_ZnamSt11align_val_t:_ZdaPvSt11align_val_tRKSt9nothrow_t" ,
+	   "_ZnamSt11align_val_tRKSt9nothrow_t:_ZdaPvmSt11align_val_t",
+	   "_ZnamSt11align_val_tRKSt9nothrow_t:_ZdaPvSt11align_val_t",
+	   "_ZnamSt11align_val_tRKSt9nothrow_t:_ZdaPvSt11align_val_tRKSt9nothrow_t",
+	   NULL
+      };
+
+      for (unsigned i = 0; pairs[i] != NULL; i++)
+	valid_pairs->add (pairs[i]);
+    }
+
+  bool r = valid_pairs->contains (needle);
+  free (needle);
+  return r;
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +894,23 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      if (!valid_new_delete_pair_p (def_stmt, stmt))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
@@ -1662,6 +1739,8 @@ perform_tree_ssa_dce (bool aggressive)
   visited = BITMAP_ALLOC (NULL);
   propagate_necessity (aggressive);
   BITMAP_FREE (visited);
+  delete valid_pairs;
+  valid_pairs = NULL;
 
   something_changed |= eliminate_unnecessary_stmts ();
   something_changed |= cfg_altered;
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Marc Glisse]

On Fri, 10 Apr 2020, Martin Liška wrote:

> On 4/9/20 10:13 AM, Jonathan Wakely wrote:
>> On Thu, 9 Apr 2020 at 09:05, Marc Glisse wrote:
>>> Note that the matching is not 1-to-1. Array vs non-array and
>>> aligned vs non-aligned seem important, but sized and unsized delete can
>>> both match the same new, IIUC.
>> 
>> Right.
>> 
>>> Not sure about the nothrow versions...
>> 
>> This is valid, and mixes the nothrow new with non-nothrow delete:
>> 
>> delete new (std::nothrow) int;
>> 
>
> All right, there's a patch candidate that comes up with the list of possible 
> pairs.
> For better readability, I present demangled names:
>
> $ cat /tmp/pairs.txt | c++filt
> 	  "operator new(unsigned long):operator delete(void*)" ,
> 	  "operator new(unsigned long):operator delete(void*, unsigned long)" 
> ,
> 	  "operator new(unsigned long):operator delete(void*, std::nothrow_t 
> const&)" ,
> 	  "operator new(unsigned long, std::nothrow_t const&):operator 
> delete(void*)" ,
> 	  "operator new(unsigned long, std::nothrow_t const&):operator 
> delete(void*, unsigned long)" ,
> 	  "operator new(unsigned long, std::nothrow_t const&):operator 
> delete(void*, std::nothrow_t const&)" ,
> 	  /* non-[] operators with alignment.  */
> 	  "operator new(unsigned long, std::align_val_t):operator 
> delete(void*, unsigned long, std::align_val_t)" ,
> 	  "operator new(unsigned long, std::align_val_t):operator 
> delete(void*, std::align_val_t)" ,
> 	  "operator new(unsigned long, std::align_val_t):operator 
> delete(void*, std::align_val_t, std::nothrow_t const&)" ,
> 	  "operator new(unsigned long, std::align_val_t, std::nothrow_t 
> const&):operator delete(void*, unsigned long, std::align_val_t)" ,
> 	  "operator new(unsigned long, std::align_val_t, std::nothrow_t 
> const&):operator delete(void*, std::align_val_t)" ,
> 	  "operator new(unsigned long, std::align_val_t, std::nothrow_t 
> const&):operator delete(void*, std::align_val_t, std::nothrow_t const&)" ,
> 	  /* [] operators.  */
> 	   "operator new[](unsigned long):operator delete[](void*)" ,
> 	   "operator new[](unsigned long):operator delete[](void*, unsigned 
> long)" ,
> 	   "operator new[](unsigned long):operator delete[](void*, 
> std::nothrow_t const&)" ,
> 	   "operator new[](unsigned long, std::nothrow_t const&):operator 
> delete[](void*)" ,
> 	   "operator new[](unsigned long, std::nothrow_t const&):operator 
> delete[](void*, unsigned long)" ,
> 	   "operator new[](unsigned long, std::nothrow_t const&):operator 
> delete[](void*, std::nothrow_t const&)" ,
> 	  /* [] operators with alignment.  */
> 	   "operator new[](unsigned long, std::align_val_t):operator 
> delete[](void*, unsigned long, std::align_val_t)" ,
> 	   "operator new[](unsigned long, std::align_val_t):operator 
> delete[](void*, std::align_val_t)" ,
> 	   "operator new[](unsigned long, std::align_val_t):operator 
> delete[](void*, std::align_val_t, std::nothrow_t const&)" ,
> 	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t 
> const&):operator delete[](void*, unsigned long, std::align_val_t)",
> 	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t 
> const&):operator delete[](void*, std::align_val_t)",
> 	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t 
> const&):operator delete[](void*, std::align_val_t, std::nothrow_t const&)",
>
> Marc pointed out that some targets do not use the leading '_' (or use 2 
> dashes?) for mangled named?
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

How do you handle platforms where size_t is not unsigned long?

-- 
Marc Glisse

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Iain Sandoe]

Marc Glisse <marc.glisse@inria.fr> wrote:

> On Fri, 10 Apr 2020, Martin Liška wrote:
>
>> On 4/9/20 10:13 AM, Jonathan Wakely wrote:
>>> On Thu, 9 Apr 2020 at 09:05, Marc Glisse wrote:
>>>> Note that the matching is not 1-to-1. Array vs non-array and
>>>> aligned vs non-aligned seem important, but sized and unsized delete can
>>>> both match the same new, IIUC.
>>> Right.
>>>> Not sure about the nothrow versions...
>>> This is valid, and mixes the nothrow new with non-nothrow delete:
>>> delete new (std::nothrow) int;
>>
>> All right, there's a patch candidate that comes up with the list of  
>> possible pairs.
>> For better readability, I present demangled names:
>>
>> $ cat /tmp/pairs.txt | c++filt
>> 	  "operator new(unsigned long):operator delete(void*)" ,
>> 	  "operator new(unsigned long):operator delete(void*, unsigned long)" ,
>> 	  "operator new(unsigned long):operator delete(void*, std::nothrow_t const&)" ,
>> 	  "operator new(unsigned long, std::nothrow_t const&):operator delete(void*)" ,
>> 	  "operator new(unsigned long, std::nothrow_t const&):operator delete(void*, unsigned long)" ,
>> 	  "operator new(unsigned long, std::nothrow_t const&):operator delete(void*, std::nothrow_t const&)" ,
>> 	  /* non-[] operators with alignment.  */
>> 	  "operator new(unsigned long, std::align_val_t):operator delete(void*, unsigned long, std::align_val_t)" ,
>> 	  "operator new(unsigned long, std::align_val_t):operator delete(void*, std::align_val_t)" ,
>> 	  "operator new(unsigned long, std::align_val_t):operator delete(void*, std::align_val_t, std::nothrow_t const&)" ,
>> 	  "operator new(unsigned long, std::align_val_t, std::nothrow_t const&):operator delete(void*, unsigned long, std::align_val_t)" ,
>> 	  "operator new(unsigned long, std::align_val_t, std::nothrow_t const&):operator delete(void*, std::align_val_t)" ,
>> 	  "operator new(unsigned long, std::align_val_t, std::nothrow_t const&):operator delete(void*, std::align_val_t, std::nothrow_t const&)" ,
>> 	  /* [] operators.  */
>> 	   "operator new[](unsigned long):operator delete[](void*)" ,
>> 	   "operator new[](unsigned long):operator delete[](void*, unsigned long)" ,
>> 	   "operator new[](unsigned long):operator delete[](void*, std::nothrow_t const&)" ,
>> 	   "operator new[](unsigned long, std::nothrow_t const&):operator delete[](void*)" ,
>> 	   "operator new[](unsigned long, std::nothrow_t const&):operator delete[](void*, unsigned long)" ,
>> 	   "operator new[](unsigned long, std::nothrow_t const&):operator delete[](void*, std::nothrow_t const&)" ,
>> 	  /* [] operators with alignment.  */
>> 	   "operator new[](unsigned long, std::align_val_t):operator delete[](void*, unsigned long, std::align_val_t)" ,
>> 	   "operator new[](unsigned long, std::align_val_t):operator delete[](void*, std::align_val_t)" ,
>> 	   "operator new[](unsigned long, std::align_val_t):operator delete[](void*, std::align_val_t, std::nothrow_t const&)" ,
>> 	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t const&):operator delete[](void*, unsigned long, std::align_val_t)",
>> 	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t const&):operator delete[](void*, std::align_val_t)",
>> 	   "operator new[](unsigned long, std::align_val_t, std::nothrow_t const&):operator delete[](void*, std::align_val_t, std::nothrow_t const&)",
>>
>> Marc pointed out that some targets do not use the leading '_' (or use 2  
>> dashes?) for mangled named?

This is USER_LABEL_PREFIX.  Several targets, including Darwin, do use it.

Other than pre-pending the U_L_P, name mangling on Darwin (and I’d imagine  
any other Itanium ABI platform) is as per ABI.

>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> How do you handle platforms where size_t is not unsigned long?
>
> -- 
> Marc Glisse

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jakub Jelinek]

On Fri, Apr 10, 2020 at 10:29:29AM +0200, Martin Liška wrote:
> +/* Valid pairs of new and delete operators for DCE.  */
> +static hash_set<nofree_string_hash> *valid_pairs = NULL;
> +
> +/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
> +   and delete  operators.  */
> +
> +static bool
> +valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
> +{
> +  const char *new_name
> +    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call)));
> +  const char *delete_name
> +    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call)));
> +
> +  if (new_name[0] == '_' and new_name[1] == '_')
> +    ++new_name;
> +  if (delete_name[0] == '_' and delete_name[1] == '_')
> +    ++delete_name;
> +
> +  char *needle = concat (new_name, ":", delete_name, NULL);
> +
> +  if (valid_pairs == NULL)
> +    {
> +      valid_pairs = new hash_set<nofree_string_hash> ();
> +      /* Invalid pairs:
> +	 non-[] and []
> +	 aligned and non-aligned
> +      */
> +
> +      const char *pairs[] = {
> +	  /* non-[] operators.  */
> +	  "_Znwm:_ZdlPv" ,
> +	  "_Znwm:_ZdlPvm" ,

Formatting, I think the /* and "_Z should be two columns from const char,
and no space before ,
Not sure I like using the : and using a hash table looks like an overkill to
me, don't we have only 8 options, and most of them with different identifier
lengths?  The concat itself will take a while...
And, what's worse, the m in there is really different on different targets.
It can be j, m or y depending on what fundamental type size_t is.

So why not (completely untested):

static bool
valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
{
  tree new_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call));
  tree delete_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call));
  const char *new_name = IDENTIFIER_POINTER (new_asm);
  const char *delete_name = IDENTIFIER_POINTER (delete_asm);
  unsigned int new_len = IDENTIFIER_LENGTH (mew_asm);
  unsigned int delete_len = IDENTIFIER_LENGTH (delete_asm);
  if (new_name < 5 || delete_len < 6)
    return false;
  if (new_name[0] == '_')
    ++new_name, --new_len;
  if (new_name[0] == '_')
    ++new_name, --new_len;
  if (delete_name[0] == '_')
    ++delete_name, --delete_len;
  if (delete_name[0] == '_')
    ++delete_name, --delete_len;
  if (new_len < 4 || delete_len < 5)
    return false;
  /* *_len is now just the length after initial underscores.  */
  if (new_name[0] != 'Z' || new_name[1] != 'n')
    return false;
  if (delete_name[0] != 'Z' || delete_name[1] != 'd')
    return false;
  /* _Znw must match _Zdl, _Zna must match _Zda.  */
  if ((new_name[2] != 'w' || delete_name[2] != 'l')
      && (new_name[2] != 'a' || delete_name[2] != 'a'))
    return false;
  if (new_name[3] != 'j' && new_name[3] != 'm' && new_name[3] != 'y')
    return false;
  if (delete_name[3] != 'P' || delete_name[4] != 'v')
    return false;
  if (new_len == 4
      || (new_len == 18 && !memcmp (new_name + 4, "RKSt9nothrow_t", 14)))
    {
      /* _ZnXY or _ZnXYRKSt9nothrow_t matches
	 _ZdXPv, _ZdXPvY and _ZdXPvRKSt9nothrow_t.  */
      if (delete_len == 5)
	return true;
      if (delete_len == 6 && delete_name[5] == new_name[3])
	return true;
      if (delete_len == 19 && !memcmp (delete_name + 5, "RKSt9nothrow_t", 14))
	return true;
    }
  else if ((new_len == 19 && !memcmp (new_name + 4, "St11align_val_t", 15))
	   || (new_len == 33
	       && !memcmp (new_name + 4, "St11align_val_tRKSt9nothrow_t", 29)))
    {
      /* _ZnXYSt11align_val_t or _ZnXYSt11align_val_tRKSt9nothrow_t matches
	 _ZdXPvSt11align_val_t or _ZdXPvYSt11align_val_t or  or
	 _ZdXPvSt11align_val_tRKSt9nothrow_t.  */
      if (delete_len == 20 && !memcmp (delete_name + 5, "St11align_val_t", 15))
	return true;
      if (delete_len == 21
	  && delete_name[5] == new_name[3]
	  && !memcmp (delete_name + 6, "St11align_val_t", 15))
	return true;
      if (delete_len == 34
	  && !memcmp (delete_name + 5, "St11align_val_tRKSt9nothrow_t", 29))
	return true;
    }
  return false;
}

	Jakub

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/10/20 11:17 AM, Jakub Jelinek wrote:
> On Fri, Apr 10, 2020 at 10:29:29AM +0200, Martin Liška wrote:
>> +/* Valid pairs of new and delete operators for DCE.  */
>> +static hash_set<nofree_string_hash> *valid_pairs = NULL;
>> +
>> +/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
>> +   and delete  operators.  */
>> +
>> +static bool
>> +valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
>> +{
>> +  const char *new_name
>> +    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call)));
>> +  const char *delete_name
>> +    = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call)));
>> +
>> +  if (new_name[0] == '_' and new_name[1] == '_')
>> +    ++new_name;
>> +  if (delete_name[0] == '_' and delete_name[1] == '_')
>> +    ++delete_name;
>> +
>> +  char *needle = concat (new_name, ":", delete_name, NULL);
>> +
>> +  if (valid_pairs == NULL)
>> +    {
>> +      valid_pairs = new hash_set<nofree_string_hash> ();
>> +      /* Invalid pairs:
>> +	 non-[] and []
>> +	 aligned and non-aligned
>> +      */
>> +
>> +      const char *pairs[] = {
>> +	  /* non-[] operators.  */
>> +	  "_Znwm:_ZdlPv" ,
>> +	  "_Znwm:_ZdlPvm" ,
> 
> Formatting, I think the /* and "_Z should be two columns from const char,
> and no space before ,
> Not sure I like using the : and using a hash table looks like an overkill to
> me, don't we have only 8 options, and most of them with different identifier
> lengths?  The concat itself will take a while...
> And, what's worse, the m in there is really different on different targets.
> It can be j, m or y depending on what fundamental type size_t is.
> 
> So why not (completely untested):

Hi.

I've updated a bit the Jakub's patch.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

> 
> static bool
> valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
> {
>    tree new_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call));
>    tree delete_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call));
>    const char *new_name = IDENTIFIER_POINTER (new_asm);
>    const char *delete_name = IDENTIFIER_POINTER (delete_asm);
>    unsigned int new_len = IDENTIFIER_LENGTH (mew_asm);
>    unsigned int delete_len = IDENTIFIER_LENGTH (delete_asm);
>    if (new_name < 5 || delete_len < 6)
>      return false;
>    if (new_name[0] == '_')
>      ++new_name, --new_len;
>    if (new_name[0] == '_')
>      ++new_name, --new_len;
>    if (delete_name[0] == '_')
>      ++delete_name, --delete_len;
>    if (delete_name[0] == '_')
>      ++delete_name, --delete_len;
>    if (new_len < 4 || delete_len < 5)
>      return false;
>    /* *_len is now just the length after initial underscores.  */
>    if (new_name[0] != 'Z' || new_name[1] != 'n')
>      return false;
>    if (delete_name[0] != 'Z' || delete_name[1] != 'd')
>      return false;
>    /* _Znw must match _Zdl, _Zna must match _Zda.  */
>    if ((new_name[2] != 'w' || delete_name[2] != 'l')
>        && (new_name[2] != 'a' || delete_name[2] != 'a'))
>      return false;
>    if (new_name[3] != 'j' && new_name[3] != 'm' && new_name[3] != 'y')
>      return false;
>    if (delete_name[3] != 'P' || delete_name[4] != 'v')
>      return false;
>    if (new_len == 4
>        || (new_len == 18 && !memcmp (new_name + 4, "RKSt9nothrow_t", 14)))
>      {
>        /* _ZnXY or _ZnXYRKSt9nothrow_t matches
> 	 _ZdXPv, _ZdXPvY and _ZdXPvRKSt9nothrow_t.  */
>        if (delete_len == 5)
> 	return true;
>        if (delete_len == 6 && delete_name[5] == new_name[3])
> 	return true;
>        if (delete_len == 19 && !memcmp (delete_name + 5, "RKSt9nothrow_t", 14))
> 	return true;
>      }
>    else if ((new_len == 19 && !memcmp (new_name + 4, "St11align_val_t", 15))
> 	   || (new_len == 33
> 	       && !memcmp (new_name + 4, "St11align_val_tRKSt9nothrow_t", 29)))
>      {
>        /* _ZnXYSt11align_val_t or _ZnXYSt11align_val_tRKSt9nothrow_t matches
> 	 _ZdXPvSt11align_val_t or _ZdXPvYSt11align_val_t or  or
> 	 _ZdXPvSt11align_val_tRKSt9nothrow_t.  */
>        if (delete_len == 20 && !memcmp (delete_name + 5, "St11align_val_t", 15))
> 	return true;
>        if (delete_len == 21
> 	  && delete_name[5] == new_name[3]
> 	  && !memcmp (delete_name + 6, "St11align_val_t", 15))
> 	return true;
>        if (delete_len == 34
> 	  && !memcmp (delete_name + 5, "St11align_val_tRKSt9nothrow_t", 29))
> 	return true;
>      }
>    return false;
> }
> 
> 	Jakub
> 


[-- Attachment #2: 0001-List-valid-pairs-for-new-and-delete-operators.patch --]
[-- Type: text/x-patch, Size: 7492 bytes --]

From 60626d4b040bc502c563b7c014b62751a517705a Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 9 Apr 2020 15:50:58 +0200
Subject: [PATCH] List valid pairs for new and delete operators.

gcc/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* cgraphclones.c (set_new_clone_decl_and_node_flags): Drop
	DECL_IS_REPLACEABLE_OPERATOR during cloning.
	* tree-ssa-dce.c (valid_new_delete_pair_p): New function.
	(propagate_necessity): Check operator names.
	(perform_tree_ssa_dce): Delete valid_pairs.

gcc/testsuite/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* g++.dg/pr94314-4.C: New test.
---
 gcc/cgraphclones.c               |   2 +
 gcc/testsuite/g++.dg/pr94314-4.C |  33 ++++++++++
 gcc/tree-ssa-dce.c               | 103 ++++++++++++++++++++++++++++---
 3 files changed, 128 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-4.C

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c73b8f810f0..8f541a28b6e 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -165,6 +165,7 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
   DECL_STATIC_DESTRUCTOR (new_node->decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_node->decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_node->decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_node->decl) = 0;
 
   new_node->externally_visible = 0;
   new_node->local = 1;
@@ -1030,6 +1031,7 @@ cgraph_node::create_version_clone_with_body
   DECL_STATIC_DESTRUCTOR (new_decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_decl) = 0;
 
   /* Create the new version's call-graph node.
      and update the edges of the new node. */
diff --git a/gcc/testsuite/g++.dg/pr94314-4.C b/gcc/testsuite/g++.dg/pr94314-4.C
new file mode 100644
index 00000000000..afa2a443dc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-4.C
@@ -0,0 +1,33 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+int count = 0;
+
+__attribute__((malloc, noinline)) void* operator new[](__SIZE_TYPE__ sz) {
+  ++count;
+  return ::operator new(sz);
+}
+
+void operator delete[](void* ptr) noexcept {
+  --count;
+  ::operator delete(ptr);
+}
+
+void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
+  --count;
+  ::operator delete(ptr, sz);
+}
+
+int main() {
+  delete[] new int[1];
+  if (count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index fd5f24c746c..69566cd50e2 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,80 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Valid pairs of new and delete operators for DCE.  */
+static hash_set<nofree_string_hash> *valid_pairs = NULL;
+
+/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
+   and delete  operators.  */
+
+static bool
+valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
+{
+  tree new_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call));
+  tree delete_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call));
+  const char *new_name = IDENTIFIER_POINTER (new_asm);
+  const char *delete_name = IDENTIFIER_POINTER (delete_asm);
+  unsigned int new_len = IDENTIFIER_LENGTH (new_asm);
+  unsigned int delete_len = IDENTIFIER_LENGTH (delete_asm);
+
+  if (new_len < 5 || delete_len < 6)
+    return false;
+  if (new_name[0] == '_')
+    ++new_name, --new_len;
+  if (new_name[0] == '_')
+    ++new_name, --new_len;
+  if (delete_name[0] == '_')
+    ++delete_name, --delete_len;
+  if (delete_name[0] == '_')
+    ++delete_name, --delete_len;
+  if (new_len < 4 || delete_len < 5)
+    return false;
+  /* *_len is now just the length after initial underscores.  */
+  if (new_name[0] != 'Z' || new_name[1] != 'n')
+    return false;
+  if (delete_name[0] != 'Z' || delete_name[1] != 'd')
+    return false;
+  /* _Znw must match _Zdl, _Zna must match _Zda.  */
+  if ((new_name[2] != 'w' || delete_name[2] != 'l')
+      && (new_name[2] != 'a' || delete_name[2] != 'a'))
+    return false;
+  /* 'j', 'm' and 'y' correspond to size_t.  */
+  if (new_name[3] != 'j' && new_name[3] != 'm' && new_name[3] != 'y')
+    return false;
+  if (delete_name[3] != 'P' || delete_name[4] != 'v')
+    return false;
+  if (new_len == 4
+      || (new_len == 18 && !memcmp (new_name + 4, "RKSt9nothrow_t", 14)))
+    {
+      /* _ZnXY or _ZnXYRKSt9nothrow_t matches
+	 _ZdXPv, _ZdXPvY and _ZdXPvRKSt9nothrow_t.  */
+      if (delete_len == 5)
+	return true;
+      if (delete_len == 6 && delete_name[5] == new_name[3])
+	return true;
+      if (delete_len == 19 && !memcmp (delete_name + 5, "RKSt9nothrow_t", 14))
+	return true;
+    }
+  else if ((new_len == 19 && !memcmp (new_name + 4, "St11align_val_t", 15))
+	   || (new_len == 33
+	       && !memcmp (new_name + 4, "St11align_val_tRKSt9nothrow_t", 29)))
+    {
+      /* _ZnXYSt11align_val_t or _ZnXYSt11align_val_tRKSt9nothrow_t matches
+	 _ZdXPvSt11align_val_t or _ZdXPvYSt11align_val_t or  or
+	 _ZdXPvSt11align_val_tRKSt9nothrow_t.  */
+      if (delete_len == 20 && !memcmp (delete_name + 5, "St11align_val_t", 15))
+	return true;
+      if (delete_len == 21
+	  && delete_name[5] == new_name[3]
+	  && !memcmp (delete_name + 6, "St11align_val_t", 15))
+	return true;
+      if (delete_len == 34
+	  && !memcmp (delete_name + 5, "St11align_val_tRKSt9nothrow_t", 29))
+	return true;
+    }
+  return false;
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +898,23 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      if (!valid_new_delete_pair_p (def_stmt, stmt))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
@@ -1662,6 +1743,8 @@ perform_tree_ssa_dce (bool aggressive)
   visited = BITMAP_ALLOC (NULL);
   propagate_necessity (aggressive);
   BITMAP_FREE (visited);
+  delete valid_pairs;
+  valid_pairs = NULL;
 
   something_changed |= eliminate_unnecessary_stmts ();
   something_changed |= cfg_altered;
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/14/20 9:09 AM, Martin Liška wrote:
> I've updated a bit the Jakub's patch.

I forgot to remove valid_pairs data structure, fixed here.

Martin

[-- Attachment #2: 0001-List-valid-pairs-for-new-and-delete-operators.patch --]
[-- Type: text/x-patch, Size: 7030 bytes --]

From fb093e29d013ce0ce8e9181bd10635d14a48bcd2 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 9 Apr 2020 15:50:58 +0200
Subject: [PATCH] List valid pairs for new and delete operators.

gcc/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* cgraphclones.c (set_new_clone_decl_and_node_flags): Drop
	DECL_IS_REPLACEABLE_OPERATOR during cloning.
	* tree-ssa-dce.c (valid_new_delete_pair_p): New function.
	(propagate_necessity): Check operator names.

gcc/testsuite/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* g++.dg/pr94314-4.C: New test.
---
 gcc/cgraphclones.c               |  2 +
 gcc/testsuite/g++.dg/pr94314-4.C | 33 +++++++++++
 gcc/tree-ssa-dce.c               | 98 ++++++++++++++++++++++++++++----
 3 files changed, 123 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-4.C

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c73b8f810f0..8f541a28b6e 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -165,6 +165,7 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
   DECL_STATIC_DESTRUCTOR (new_node->decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_node->decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_node->decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_node->decl) = 0;
 
   new_node->externally_visible = 0;
   new_node->local = 1;
@@ -1030,6 +1031,7 @@ cgraph_node::create_version_clone_with_body
   DECL_STATIC_DESTRUCTOR (new_decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_decl) = 0;
 
   /* Create the new version's call-graph node.
      and update the edges of the new node. */
diff --git a/gcc/testsuite/g++.dg/pr94314-4.C b/gcc/testsuite/g++.dg/pr94314-4.C
new file mode 100644
index 00000000000..afa2a443dc4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-4.C
@@ -0,0 +1,33 @@
+/* PR c++/94314.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
+/* { dg-additional-options "-fdelete-null-pointer-checks" } */
+
+#include <stdio.h>
+
+int count = 0;
+
+__attribute__((malloc, noinline)) void* operator new[](__SIZE_TYPE__ sz) {
+  ++count;
+  return ::operator new(sz);
+}
+
+void operator delete[](void* ptr) noexcept {
+  --count;
+  ::operator delete(ptr);
+}
+
+void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
+  --count;
+  ::operator delete(ptr, sz);
+}
+
+int main() {
+  delete[] new int[1];
+  if (count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index fd5f24c746c..757cfad5b5e 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,77 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
+   and delete  operators.  */
+
+static bool
+valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
+{
+  tree new_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call));
+  tree delete_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call));
+  const char *new_name = IDENTIFIER_POINTER (new_asm);
+  const char *delete_name = IDENTIFIER_POINTER (delete_asm);
+  unsigned int new_len = IDENTIFIER_LENGTH (new_asm);
+  unsigned int delete_len = IDENTIFIER_LENGTH (delete_asm);
+
+  if (new_len < 5 || delete_len < 6)
+    return false;
+  if (new_name[0] == '_')
+    ++new_name, --new_len;
+  if (new_name[0] == '_')
+    ++new_name, --new_len;
+  if (delete_name[0] == '_')
+    ++delete_name, --delete_len;
+  if (delete_name[0] == '_')
+    ++delete_name, --delete_len;
+  if (new_len < 4 || delete_len < 5)
+    return false;
+  /* *_len is now just the length after initial underscores.  */
+  if (new_name[0] != 'Z' || new_name[1] != 'n')
+    return false;
+  if (delete_name[0] != 'Z' || delete_name[1] != 'd')
+    return false;
+  /* _Znw must match _Zdl, _Zna must match _Zda.  */
+  if ((new_name[2] != 'w' || delete_name[2] != 'l')
+      && (new_name[2] != 'a' || delete_name[2] != 'a'))
+    return false;
+  /* 'j', 'm' and 'y' correspond to size_t.  */
+  if (new_name[3] != 'j' && new_name[3] != 'm' && new_name[3] != 'y')
+    return false;
+  if (delete_name[3] != 'P' || delete_name[4] != 'v')
+    return false;
+  if (new_len == 4
+      || (new_len == 18 && !memcmp (new_name + 4, "RKSt9nothrow_t", 14)))
+    {
+      /* _ZnXY or _ZnXYRKSt9nothrow_t matches
+	 _ZdXPv, _ZdXPvY and _ZdXPvRKSt9nothrow_t.  */
+      if (delete_len == 5)
+	return true;
+      if (delete_len == 6 && delete_name[5] == new_name[3])
+	return true;
+      if (delete_len == 19 && !memcmp (delete_name + 5, "RKSt9nothrow_t", 14))
+	return true;
+    }
+  else if ((new_len == 19 && !memcmp (new_name + 4, "St11align_val_t", 15))
+	   || (new_len == 33
+	       && !memcmp (new_name + 4, "St11align_val_tRKSt9nothrow_t", 29)))
+    {
+      /* _ZnXYSt11align_val_t or _ZnXYSt11align_val_tRKSt9nothrow_t matches
+	 _ZdXPvSt11align_val_t or _ZdXPvYSt11align_val_t or  or
+	 _ZdXPvSt11align_val_tRKSt9nothrow_t.  */
+      if (delete_len == 20 && !memcmp (delete_name + 5, "St11align_val_t", 15))
+	return true;
+      if (delete_len == 21
+	  && delete_name[5] == new_name[3]
+	  && !memcmp (delete_name + 6, "St11align_val_t", 15))
+	return true;
+      if (delete_len == 34
+	  && !memcmp (delete_name + 5, "St11align_val_tRKSt9nothrow_t", 29))
+	return true;
+    }
+  return false;
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +895,23 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      if (!valid_new_delete_pair_p (def_stmt, stmt))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jakub Jelinek]

On Tue, Apr 14, 2020 at 09:11:37AM +0200, Martin Liška wrote:
> +/* PR c++/94314.  */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
> +/* { dg-additional-options "-fdelete-null-pointer-checks" } */

Any reason why you want to do it for -std=c++14 only?
Wouldn't
// PR c++/94314
// { dg-do run { target c++14 } }
// { dg-options "-O2 -fdelete-null-pointer-checks -fdump-tree-cddce-details" }
be better (and no need for dg-additional-options if you have dg-options
already and don't have some effective target on it).

> +
> +#include <stdio.h>

What do you need stdio.h for?

Otherwise, LGTM, but please give the C++ maintainers time to comment.

	Jakub

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Martin Liška]

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

On 4/14/20 10:37 AM, Jakub Jelinek wrote:
> On Tue, Apr 14, 2020 at 09:11:37AM +0200, Martin Liška wrote:
>> +/* PR c++/94314.  */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
>> +/* { dg-additional-options "-fdelete-null-pointer-checks" } */
> 
> Any reason why you want to do it for -std=c++14 only?

I need at least -std=c++11 for the noexcept keyword. I updated that.

> Wouldn't
> // PR c++/94314
> // { dg-do run { target c++14 } }
> // { dg-options "-O2 -fdelete-null-pointer-checks -fdump-tree-cddce-details" }
> be better (and no need for dg-additional-options if you have dg-options
> already and don't have some effective target on it).

Sure, removed that.

> 
>> +
>> +#include <stdio.h>
> 
> What do you need stdio.h for?

Likewise here (also for other test-cases).

> 
> Otherwise, LGTM, but please give the C++ maintainers time to comment.

Sure.
Martin

> 
> 	Jakub
> 


[-- Attachment #2: 0001-List-valid-pairs-for-new-and-delete-operators.patch --]
[-- Type: text/x-patch, Size: 8812 bytes --]

From 29626be765eea010f7d1505d08d594844e4518bf Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 9 Apr 2020 15:50:58 +0200
Subject: [PATCH] List valid pairs for new and delete operators.

gcc/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* cgraphclones.c (set_new_clone_decl_and_node_flags): Drop
	DECL_IS_REPLACEABLE_OPERATOR during cloning.
	* tree-ssa-dce.c (valid_new_delete_pair_p): New function.
	(propagate_necessity): Check operator names.

gcc/testsuite/ChangeLog:

2020-04-09  Martin Liska  <mliska@suse.cz>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* g++.dg/pr94314.C: Do not use dg-additional-options
	and remove not needed stdio.h include.
	* g++.dg/pr94314-2.C: Likewise.
	* g++.dg/pr94314-3.C: Likewise.
	* g++.dg/pr94314-4.C: New test.
---
 gcc/cgraphclones.c               |  2 +
 gcc/testsuite/g++.dg/pr94314-2.C |  5 +-
 gcc/testsuite/g++.dg/pr94314-3.C |  5 +-
 gcc/testsuite/g++.dg/pr94314-4.C | 30 ++++++++++
 gcc/testsuite/g++.dg/pr94314.C   |  5 +-
 gcc/tree-ssa-dce.c               | 98 ++++++++++++++++++++++++++++----
 6 files changed, 123 insertions(+), 22 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr94314-4.C

diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index c73b8f810f0..8f541a28b6e 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -165,6 +165,7 @@ set_new_clone_decl_and_node_flags (cgraph_node *new_node)
   DECL_STATIC_DESTRUCTOR (new_node->decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_node->decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_node->decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_node->decl) = 0;
 
   new_node->externally_visible = 0;
   new_node->local = 1;
@@ -1030,6 +1031,7 @@ cgraph_node::create_version_clone_with_body
   DECL_STATIC_DESTRUCTOR (new_decl) = 0;
   DECL_SET_IS_OPERATOR_NEW (new_decl, 0);
   DECL_SET_IS_OPERATOR_DELETE (new_decl, 0);
+  DECL_IS_REPLACEABLE_OPERATOR (new_decl) = 0;
 
   /* Create the new version's call-graph node.
      and update the edges of the new node. */
diff --git a/gcc/testsuite/g++.dg/pr94314-2.C b/gcc/testsuite/g++.dg/pr94314-2.C
index 36b93ed6d4d..998ce601767 100644
--- a/gcc/testsuite/g++.dg/pr94314-2.C
+++ b/gcc/testsuite/g++.dg/pr94314-2.C
@@ -1,9 +1,6 @@
 /* PR c++/94314.  */
 /* { dg-do run } */
-/* { dg-options "-O2 -fdump-tree-cddce-details" } */
-/* { dg-additional-options "-fdelete-null-pointer-checks" } */
-
-#include <stdio.h>
+/* { dg-options "-O2 -fdump-tree-cddce-details -fdelete-null-pointer-checks" } */
 
 struct A
 {
diff --git a/gcc/testsuite/g++.dg/pr94314-3.C b/gcc/testsuite/g++.dg/pr94314-3.C
index 575ba9d8ad8..846a5d6a3d8 100644
--- a/gcc/testsuite/g++.dg/pr94314-3.C
+++ b/gcc/testsuite/g++.dg/pr94314-3.C
@@ -1,9 +1,6 @@
 /* PR c++/94314.  */
 /* { dg-do run } */
-/* { dg-options "-O2 --param early-inlining-insns=100 -fdump-tree-cddce-details" } */
-/* { dg-additional-options "-fdelete-null-pointer-checks" } */
-
-#include <stdio.h>
+/* { dg-options "-O2 --param early-inlining-insns=100 -fdump-tree-cddce-details -fdelete-null-pointer-checks" } */
 
 volatile int idx;
 
diff --git a/gcc/testsuite/g++.dg/pr94314-4.C b/gcc/testsuite/g++.dg/pr94314-4.C
new file mode 100644
index 00000000000..d097f29d4ad
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr94314-4.C
@@ -0,0 +1,30 @@
+/* PR c++/94314.  */
+/* { dg-do run { target c++11 } } */
+/* { dg-options "-O2 -fdump-tree-cddce-details -fdelete-null-pointer-checks" } */
+
+int count = 0;
+
+__attribute__((malloc, noinline)) void* operator new[](__SIZE_TYPE__ sz) {
+  ++count;
+  return ::operator new(sz);
+}
+
+void operator delete[](void* ptr) noexcept {
+  --count;
+  ::operator delete(ptr);
+}
+
+void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
+  --count;
+  ::operator delete(ptr, sz);
+}
+
+int main() {
+  delete[] new int[1];
+  if (count != 0)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Deleting : operator delete" "cddce1"} } */
diff --git a/gcc/testsuite/g++.dg/pr94314.C b/gcc/testsuite/g++.dg/pr94314.C
index 86e651d10ba..4e5ae122e9f 100644
--- a/gcc/testsuite/g++.dg/pr94314.C
+++ b/gcc/testsuite/g++.dg/pr94314.C
@@ -1,9 +1,6 @@
 /* PR c++/94314.  */
 /* { dg-do run } */
-/* { dg-options "-O2 -fdump-tree-cddce-details" } */
-/* { dg-additional-options "-fdelete-null-pointer-checks" } */
-
-#include <stdio.h>
+/* { dg-options "-O2 -fdump-tree-cddce-details -fdelete-null-pointer-checks" } */
 
 struct A
 {
diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index fd5f24c746c..757cfad5b5e 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -646,6 +646,77 @@ degenerate_phi_p (gimple *phi)
   return true;
 }
 
+/* Return that NEW_CALL and DELETE_CALL are a valid pair of new
+   and delete  operators.  */
+
+static bool
+valid_new_delete_pair_p (gimple *new_call, gimple *delete_call)
+{
+  tree new_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (new_call));
+  tree delete_asm = DECL_ASSEMBLER_NAME (gimple_call_fndecl (delete_call));
+  const char *new_name = IDENTIFIER_POINTER (new_asm);
+  const char *delete_name = IDENTIFIER_POINTER (delete_asm);
+  unsigned int new_len = IDENTIFIER_LENGTH (new_asm);
+  unsigned int delete_len = IDENTIFIER_LENGTH (delete_asm);
+
+  if (new_len < 5 || delete_len < 6)
+    return false;
+  if (new_name[0] == '_')
+    ++new_name, --new_len;
+  if (new_name[0] == '_')
+    ++new_name, --new_len;
+  if (delete_name[0] == '_')
+    ++delete_name, --delete_len;
+  if (delete_name[0] == '_')
+    ++delete_name, --delete_len;
+  if (new_len < 4 || delete_len < 5)
+    return false;
+  /* *_len is now just the length after initial underscores.  */
+  if (new_name[0] != 'Z' || new_name[1] != 'n')
+    return false;
+  if (delete_name[0] != 'Z' || delete_name[1] != 'd')
+    return false;
+  /* _Znw must match _Zdl, _Zna must match _Zda.  */
+  if ((new_name[2] != 'w' || delete_name[2] != 'l')
+      && (new_name[2] != 'a' || delete_name[2] != 'a'))
+    return false;
+  /* 'j', 'm' and 'y' correspond to size_t.  */
+  if (new_name[3] != 'j' && new_name[3] != 'm' && new_name[3] != 'y')
+    return false;
+  if (delete_name[3] != 'P' || delete_name[4] != 'v')
+    return false;
+  if (new_len == 4
+      || (new_len == 18 && !memcmp (new_name + 4, "RKSt9nothrow_t", 14)))
+    {
+      /* _ZnXY or _ZnXYRKSt9nothrow_t matches
+	 _ZdXPv, _ZdXPvY and _ZdXPvRKSt9nothrow_t.  */
+      if (delete_len == 5)
+	return true;
+      if (delete_len == 6 && delete_name[5] == new_name[3])
+	return true;
+      if (delete_len == 19 && !memcmp (delete_name + 5, "RKSt9nothrow_t", 14))
+	return true;
+    }
+  else if ((new_len == 19 && !memcmp (new_name + 4, "St11align_val_t", 15))
+	   || (new_len == 33
+	       && !memcmp (new_name + 4, "St11align_val_tRKSt9nothrow_t", 29)))
+    {
+      /* _ZnXYSt11align_val_t or _ZnXYSt11align_val_tRKSt9nothrow_t matches
+	 _ZdXPvSt11align_val_t or _ZdXPvYSt11align_val_t or  or
+	 _ZdXPvSt11align_val_tRKSt9nothrow_t.  */
+      if (delete_len == 20 && !memcmp (delete_name + 5, "St11align_val_t", 15))
+	return true;
+      if (delete_len == 21
+	  && delete_name[5] == new_name[3]
+	  && !memcmp (delete_name + 6, "St11align_val_t", 15))
+	return true;
+      if (delete_len == 34
+	  && !memcmp (delete_name + 5, "St11align_val_tRKSt9nothrow_t", 29))
+	return true;
+    }
+  return false;
+}
+
 /* Propagate necessity using the operands of necessary statements.
    Process the uses on each statement in the worklist, and add all
    feeding statements which contribute to the calculation of this
@@ -824,16 +895,23 @@ propagate_necessity (bool aggressive)
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
 		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
 		{
-		  /* Delete operators can have alignment and (or) size as next
-		     arguments.  When being a SSA_NAME, they must be marked
-		     as necessary.  */
-		  if (is_delete_operator && gimple_call_num_args (stmt) >= 2)
-		    for (unsigned i = 1; i < gimple_call_num_args (stmt); i++)
-		      {
-			tree arg = gimple_call_arg (stmt, i);
-			if (TREE_CODE (arg) == SSA_NAME)
-			  mark_operand_necessary (arg);
-		      }
+		  if (is_delete_operator)
+		    {
+		      if (!valid_new_delete_pair_p (def_stmt, stmt))
+			mark_operand_necessary (gimple_call_arg (stmt, 0));
+
+		      /* Delete operators can have alignment and (or) size
+			 as next arguments.  When being a SSA_NAME, they
+			 must be marked as necessary.  */
+		      if (gimple_call_num_args (stmt) >= 2)
+			for (unsigned i = 1; i < gimple_call_num_args (stmt);
+			     i++)
+			  {
+			    tree arg = gimple_call_arg (stmt, i);
+			    if (TREE_CODE (arg) == SSA_NAME)
+			      mark_operand_necessary (arg);
+			  }
+		    }
 
 		  continue;
 		}
-- 
2.26.0

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jakub Jelinek]

On Tue, Apr 14, 2020 at 12:54:17PM +0200, Martin Liška wrote:
> On 4/14/20 10:37 AM, Jakub Jelinek wrote:
> > On Tue, Apr 14, 2020 at 09:11:37AM +0200, Martin Liška wrote:
> > > +/* PR c++/94314.  */
> > > +/* { dg-do run } */
> > > +/* { dg-options "-O2 -fdump-tree-cddce-details -std=c++14" } */
> > > +/* { dg-additional-options "-fdelete-null-pointer-checks" } */
> > 
> > Any reason why you want to do it for -std=c++14 only?
> 
> I need at least -std=c++11 for the noexcept keyword. I updated that.

With c++11 one gets:
Excess errors:
.../testsuite/g++.dg/pr94314-4.C:19:28: error: too many arguments to function 'void operator delete(void*)'
because C++ sized deallocation is a C++14 feature.

One needs to use check-c++-all or GXX_TESTSUITE_STDS=98,11,14,17,2a make check
or similar to get that though, because 11 isn't tested by default, only 98,
14 and 17 are ATM I think.

Fixed thusly, committed to trunk as obvious.

Note for next time, there shouldn't be any tests directly in g++.dg/
which has subdirectories, so optimization related tests should go into
g++.dg/opt/ etc.

2020-04-17  Jakub Jelinek  <jakub@redhat.com>

	PR c++/94314
	* g++.dg/pr94314-4.C: Require c++14 rather than c++11.

--- gcc/testsuite/g++.dg/pr94314-4.C.jj	2020-04-17 08:49:49.129682532 +0200
+++ gcc/testsuite/g++.dg/pr94314-4.C	2020-04-17 08:57:24.836861755 +0200
@@ -1,5 +1,5 @@
 /* PR c++/94314.  */
-/* { dg-do run { target c++11 } } */
+/* { dg-do run { target c++14 } } */
 /* { dg-options "-O2 -fdump-tree-cddce-details -fdelete-null-pointer-checks" } */
 
 int count = 0;

	Jakub

Re: [PATCH] Allow new/delete operator deletion only for replaceable.

[Jonathan Wakely]

On Fri, 17 Apr 2020 at 08:05, Jakub Jelinek wrote:
> One needs to use check-c++-all or GXX_TESTSUITE_STDS=98,11,14,17,2a make check
> or similar to get that though, because 11 isn't tested by default, only 98,
> 14 and 17 are ATM I think.

Right.

> Fixed thusly, committed to trunk as obvious.

Alternatively, leave it so it can be tested with C++11 but only
declare the sized delete when the compiler supports it:

#if __cpp_sized_deallocation
void operator delete[](void* ptr, __SIZE_TYPE__ sz) noexcept {
  --count;
  ::operator delete(ptr, sz);
}
#endif