Re: [RFC][PATCH] Extend DCE to remove unnecessary new/delete-pairs

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Nov 21, 2017 at 12:14 PM, Dominik Inführ
<dominik.infuehr@theobroma-systems.com> wrote:
> Hi,
>
> this patch tries to extend tree-ssa-dce.c to remove unnecessary new/delete-pairs (it already does that for malloc/free). Clang does it too and it seems to be allowed by http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3664.html. I’ve bootstrapped/regtested on aarch64-linux and x86_64-linux.

Few comments.

+/* Return true when STMT is operator new call.  */
+
+bool
+gimple_call_operator_new_p (const gimple *stmt)
+{
+  tree fndecl;
+
+  if (is_gimple_call (stmt)
+      && (fndecl = gimple_call_fndecl (stmt)) != NULL_TREE)
+    return DECL_IS_OPERATOR_NEW (fndecl);
+  return false;

make these take a gcall * please and drop the is_gimple_call check.

diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index aa54221253c..9a406c5d86c 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1787,7 +1787,9 @@ struct GTY(()) tree_function_decl {
   unsigned has_debug_args_flag : 1;
   unsigned tm_clone_flag : 1;
   unsigned versioned_function : 1;
-  /* No bits left.  */
+
+  unsigned operator_delete_flag : 1;
+  /* 31 bits left.  */

while it looks bad reality is that on 64bit pointer hosts we had 32 bits left.

Note that we could really pack things better and even make function_decl
smaller by using the tail-padding (for 64bit pointer hosts...) in
tree_decl_with_vis
which also has 32 + 14 bits left.

+                     /* For operator delete [] we need to keep size operand
+                        alive. Otherwise this operand isn't available anymore
+                        when we finally decide that this stmt is necessary in
+                       eliminate_unnecessary_stmts. If it should really be
+                       unnecessary, a later pass can clean this up.  */
+                     if (gimple_call_operator_delete_p (stmt))
+                       {
+                         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);
+                           }
+                       }

bah ... so the "hack" for malloc/free pair removal doesn't really work very well
for new/delete.  What you do looks reasonable but in the end we might support
sth like "tentatively not necessary" and a late marking things necessary that
have been proven to be necessary anyway.

diff --git a/libstdc++-v3/testsuite/ext/bitmap_allocator/check_delete.cc
b/libstdc++-v3/testsuite/ext/bitmap_allocator/check_delete.cc
index bc36ed595e0..10e26615fb7 100644
--- a/libstdc++-v3/testsuite/ext/bitmap_allocator/check_delete.cc
+++ b/libstdc++-v3/testsuite/ext/bitmap_allocator/check_delete.cc
@@ -15,6 +15,8 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.

+// { dg-options "-fno-tree-dce" }
+
 // 20.4.1.1 allocator members

hmm.  I think it would be better to add a new C++ FE option
-fno-allocation-dce that
simply doesn't set DECL_IS_OPERATOR_{NEW,DELETE} similar to how we have
-fno-lifetime-dse?  There might be projects that are not happy with this DCE and
disabling _all_ DCE just to keep existing behavior looks bad.

Otherwise the patch looks straight-forward in case C++ folks are happy with it.

Thanks for working on this!  I think that if you manage to convince C++ folks
and fix the -fno-allocation-dce missing then I'd like to have this in
early stage3.
We've been requesting this for a long time.

Richard.


> Best,
> Dominik
>