This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Detect not-cloned new/delete operators in DCE.


On 8/6/19 2:42 PM, Martin Liška wrote:
> On 8/5/19 3:46 PM, Marc Glisse wrote:
>> On Mon, 5 Aug 2019, Martin Liška wrote:
>>
>>> You are right. It can really lead to confusion of the DCE.
>>>
>>> What we have is DECL_ABSTRACT_ORIGIN(decl) which we can use to indicate operators
>>> that were somehow modified by an IPA optimization.
>>
>> Looks similar to the cgraph_node->clone_of that Richard was talking about. I have no idea which one would be best.
> 
> 
> Hm, strange that the ISRA clones don't have n->clone_of set. It's created here:
> 
> #0  cgraph_node::create (decl=<function_decl 0x7ffff721c300 _ZN1AdlEPvd.isra.0>) at /home/marxin/Programming/gcc/gcc/profile-count.h:751
> #1  0x0000000000bc8342 in cgraph_node::create_version_clone (this=<cgraph_node * const 0x7ffff7208000 "operator delete"/64>, new_decl=<optimized out>, redirect_callers=..., bbs_to_copy=0x0, suffix=0x1b74427 "isra") at /home/marxin/Programming/gcc/gcc/cgraphclones.c:960
> #2  0x0000000000bc9b9a in cgraph_node::create_version_clone_with_body (this=this@entry=<cgraph_node * const 0x7ffff7208000 "operator delete"/64>, redirect_callers=redirect_callers@entry=..., tree_map=tree_map@entry=0x0, args_to_skip=args_to_skip@entry=0x0, 
>     skip_return=skip_return@entry=false, bbs_to_copy=bbs_to_copy@entry=0x0, new_entry_block=<optimized out>, suffix=<optimized out>, target_attributes=<optimized out>) at /home/marxin/Programming/gcc/gcc/cgraphclones.c:1071
> #3  0x00000000010e4611 in modify_function (adjustments=..., node=<cgraph_node * 0x7ffff7208000 "operator delete"/64>) at /home/marxin/Programming/gcc/gcc/tree-sra.c:5493
> #4  ipa_early_sra () at /home/marxin/Programming/gcc/gcc/tree-sra.c:5731
> #5  (anonymous namespace)::pass_early_ipa_sra::execute (this=<optimized out>) at /home/marxin/Programming/gcc/gcc/tree-sra.c:5778
> 
> @Martin, @Honza: Why do we not set clone_of in this transformation?

If I'm correct cgraph_node::clone is used for inline clones only?

Anyway, I'm sending patch that considers only such new/delete operators
that are not a clone of an original type. That should make the current
DCE code more solid.

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

Ready to be installed?
Thanks,
Martin

> 
>>
>>> Do you believe it will be sufficient?
>>
>> In DCE only consider the operator delete that are not clones? (possibly the same for operator new? I don't know how much we can change the return value of a function in a clone) I guess that should work, and it wouldn't impact the common case with default, global operator new/delete.
> 
> I tent to limit that the only cgraph nodes that are not clones. I'm going to prepare a patch for it.
> 
>>
>> An alternative would be to clear the DECL_IS_OPERATOR_DELETE flag when creating a clone (possibly only if it modified the first parameter?). There is probably not much information in the fact that a function that doesn't even take a pointer used to be an operator delete.
>>
>>
>> By the way, I just thought of something, now that we handle class-specific operator new/delete (but you could do the same with the global replacable ones as well):
>>
>> #include <stdio.h>
>> int count = 0;
>> 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);}
>> };
>> int main(){
>>   delete new A;
>>   printf("%d\n",count);
>> }
>>
>> If we do not inline anything, we can remove the pair and nothing touches count. If we inline both new and delete, we can then remove the inner pair instead, count increases and decreases, fine. If we inline only one of them, and DCE the mismatched pair new/delete, we get something inconsistent (count is -1).
>>
>> This seems to indicate we should check that the new and delete match somehow...
>>
> 

>From 94110a47f0c13424d2e39d8f14bcc896a6097bd3 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 6 Aug 2019 16:14:48 +0200
Subject: [PATCH] Detect not-cloned new/delete operators in DCE.

gcc/ChangeLog:

2019-08-06  Martin Liska  <mliska@suse.cz>

	* gimple.c (gimple_call_operator_delete_p): Remove.
	* gimple.h (gimple_call_operator_delete_p): Likewise.
	* tree-ssa-dce.c (operator_new_candidate_p): New.
	(operator_delete_candidate_p): Likewise.
	(mark_stmt_if_obviously_necessary): Use operator_new_candidate_p
	and operator_delete_candidate_p in order to detect operators
	that are not not clones.
	(mark_all_reaching_defs_necessary_1): Likewise.
	(propagate_necessity): Likewise.
	(eliminate_unnecessary_stmts): Likewise.
---
 gcc/gimple.c       | 12 ----------
 gcc/gimple.h       |  1 -
 gcc/tree-ssa-dce.c | 59 ++++++++++++++++++++++++++--------------------
 3 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/gcc/gimple.c b/gcc/gimple.c
index 633ef512a19..684b8831b4d 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2707,18 +2707,6 @@ gimple_builtin_call_types_compatible_p (const gimple *stmt, tree fndecl)
   return true;
 }
 
-/* Return true when STMT is operator delete call.  */
-
-bool
-gimple_call_operator_delete_p (const gcall *stmt)
-{
-  tree fndecl;
-
-  if ((fndecl = gimple_call_fndecl (stmt)) != NULL_TREE)
-    return DECL_IS_OPERATOR_DELETE_P (fndecl);
-  return false;
-}
-
 /* Return true when STMT is builtins call.  */
 
 bool
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 55f5d0d33d9..7a1e1f49099 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1548,7 +1548,6 @@ 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_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/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index afb7bd9dedc..05d30e6c839 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -114,6 +114,25 @@ static bool cfg_altered;
 /* When non-NULL holds map from basic block index into the postorder.  */
 static int *bb_postorder;
 
+/* Return true when FNDECL is a new operator and not a clone.  */
+
+static bool
+operator_new_candidate_p (tree fndecl)
+{
+  return (fndecl != NULL_TREE
+	  && DECL_IS_OPERATOR_NEW_P (fndecl)
+	  && DECL_ABSTRACT_ORIGIN (fndecl) == NULL_TREE);
+}
+
+/* Return true when FNDECL is a delete operator and not a clone.  */
+
+static bool
+operator_delete_candidate_p (tree fndecl)
+{
+  return (fndecl != NULL_TREE
+	  && DECL_IS_OPERATOR_DELETE_P (fndecl)
+	  && DECL_ABSTRACT_ORIGIN (fndecl) == NULL_TREE);
+}
 
 /* True if we should treat any stmt with a vdef as necessary.  */
 
@@ -248,7 +267,7 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
 
 	if (callee != NULL_TREE
 	    && flag_allocation_dce
-	    && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee))
+	    && operator_new_candidate_p (callee))
 	  return;
 
 	/* Most, but not all function calls are required.  Function calls that
@@ -613,8 +632,8 @@ 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)))
+	  && (operator_new_candidate_p (callee)
+	      || operator_delete_candidate_p (callee)))
 	return false;
     }
 
@@ -806,15 +825,10 @@ propagate_necessity (bool aggressive)
 	     processing the argument.  */
 	  bool is_delete_operator
 	    = (is_gimple_call (stmt)
-	       && gimple_call_operator_delete_p (as_a <gcall *> (stmt)));
+	       && operator_delete_candidate_p (gimple_call_fndecl (as_a <gcall *> (stmt))));
 	  if (is_delete_operator
 	      || gimple_call_builtin_p (stmt, BUILT_IN_FREE))
 	    {
-	      /* It can happen that a user delete operator has the pointer
-		 argument optimized out already.  */
-	      if (gimple_call_num_args (stmt) == 0)
-		continue;
-
 	      tree ptr = gimple_call_arg (stmt, 0);
 	      gimple *def_stmt;
 	      tree def_callee;
@@ -827,7 +841,7 @@ propagate_necessity (bool aggressive)
 		       && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_ALIGNED_ALLOC
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC
 			   || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC))
-		      || DECL_IS_REPLACEABLE_OPERATOR_NEW_P (def_callee)))
+		      || operator_new_candidate_p (def_callee)))
 		{
 		  /* Delete operators can have alignment and (or) size as next
 		     arguments.  When being a SSA_NAME, they must be marked
@@ -900,8 +914,8 @@ propagate_necessity (bool aggressive)
 		continue;
 
 	      if (callee != NULL_TREE
-		  && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (callee)
-		      || DECL_IS_OPERATOR_DELETE_P (callee)))
+		  && (operator_new_candidate_p (callee)
+		      || operator_delete_candidate_p (callee)))
 		continue;
 
 	      /* Calls implicitly load from memory, their arguments
@@ -1326,20 +1340,15 @@ 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)))))
+		      && operator_delete_candidate_p (gimple_call_fndecl (as_a <gcall *> (stmt))))))
 	    {
-	      /* It can happen that a user delete operator has the pointer
-		 argument optimized out already.  */
-	      if (gimple_call_num_args (stmt) > 0)
+	      tree ptr = gimple_call_arg (stmt, 0);
+	      if (TREE_CODE (ptr) == SSA_NAME)
 		{
-		  tree ptr = gimple_call_arg (stmt, 0);
-		  if (TREE_CODE (ptr) == SSA_NAME)
-		    {
-		      gimple *def_stmt = SSA_NAME_DEF_STMT (ptr);
-		      if (!gimple_nop_p (def_stmt)
-			  && !gimple_plf (def_stmt, STMT_NECESSARY))
-			gimple_set_plf (stmt, STMT_NECESSARY, false);
-		    }
+		  gimple *def_stmt = SSA_NAME_DEF_STMT (ptr);
+		  if (!gimple_nop_p (def_stmt)
+		      && !gimple_plf (def_stmt, STMT_NECESSARY))
+		    gimple_set_plf (stmt, STMT_NECESSARY, false);
 		}
 	    }
 
@@ -1394,7 +1403,7 @@ eliminate_unnecessary_stmts (void)
 			       && DECL_FUNCTION_CODE (call) != BUILT_IN_CALLOC
 			       && !ALLOCA_FUNCTION_CODE_P
 			       (DECL_FUNCTION_CODE (call))))
-			  && !DECL_IS_REPLACEABLE_OPERATOR_NEW_P (call))))
+			  && !operator_new_candidate_p (call))))
 		{
 		  something_changed = true;
 		  if (dump_file && (dump_flags & TDF_DETAILS))
-- 
2.22.0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]