diff mbox

[PR,55264] Do not remove as unreachable any virtual methods before inlining

Message ID 20130116184224.GA26626@virgil.suse
State New
Headers show

Commit Message

Martin Jambor Jan. 16, 2013, 6:42 p.m. UTC 
On Wed, Jan 16, 2013 at 01:44:20PM +0100, Jan Hubicka wrote:
> > Perhaps could you first change cgraph_non_local_node_p_1 and try to check some code
> > if codegen differs significantly? It should not at all.
> > ipa-cp is the sole user of this flag in IPA passes, so you should know what it does.
> 
> Thinking deeper of ipa-cp and local virtuals, I think this is all slipperly.
> Local means that all calls to the functions are explicit and known. Obviously
> if function is virutal and new calls may appear by devirtualization, the local
> flag is bogus.  I guess the external functions are the only that may be local
> and virtual because somewhere there must be a vtable reference, but to play
> safe, I would suggest marking all virtuals non-local.
> 

Right, as discussed on IRC, the patch below therfore modifies
cgraph_only_called_directly_or_aliased_p to return false for virtual
functions (which translates into cleared local flag) and the cloning
machinery to clear that flag.

Bootstrapped and tested on x86_64-linux without any problems.  OK for
trunk?

Thanks,

Martin


2013-01-16  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimizations/55264
	* ipa-inline-transform.c (can_remove_node_now_p_1): Never return true
	for virtual methods.
	* ipa.c (symtab_remove_unreachable_nodes): Never return true for
	virtual methods before inlining is over.
	* cgraph.h (cgraph_only_called_directly_or_aliased_p): Return false for
	virtual functions.
	* cgraphclones.c (cgraph_create_virtual_clone): Mark clones as
	non-virtual.

testsuite/
	* g++.dg/ipa/pr55264.C: New test.

Comments

Jan Hubicka Jan. 16, 2013, 9:24 p.m. UTC | #1 
> On Wed, Jan 16, 2013 at 01:44:20PM +0100, Jan Hubicka wrote:
> > > Perhaps could you first change cgraph_non_local_node_p_1 and try to check some code
> > > if codegen differs significantly? It should not at all.
> > > ipa-cp is the sole user of this flag in IPA passes, so you should know what it does.
> > 
> > Thinking deeper of ipa-cp and local virtuals, I think this is all slipperly.
> > Local means that all calls to the functions are explicit and known. Obviously
> > if function is virutal and new calls may appear by devirtualization, the local
> > flag is bogus.  I guess the external functions are the only that may be local
> > and virtual because somewhere there must be a vtable reference, but to play
> > safe, I would suggest marking all virtuals non-local.
> > 
> 
> Right, as discussed on IRC, the patch below therfore modifies
> cgraph_only_called_directly_or_aliased_p to return false for virtual
> functions (which translates into cleared local flag) and the cloning
> machinery to clear that flag.
> 
> Bootstrapped and tested on x86_64-linux without any problems.  OK for
> trunk?
OK, thanks!

Honza
diff mbox

Patch

Index: src/gcc/ipa-inline-transform.c
===================================================================
--- src.orig/gcc/ipa-inline-transform.c
+++ src/gcc/ipa-inline-transform.c
@@ -92,9 +92,7 @@  can_remove_node_now_p_1 (struct cgraph_n
 	     those only after all devirtualizable virtual calls are processed.
 	     Lacking may edges in callgraph we just preserve them post
 	     inlining.  */
-	  && (!DECL_VIRTUAL_P (node->symbol.decl)
-	      || (!DECL_COMDAT (node->symbol.decl)
-		  && !DECL_EXTERNAL (node->symbol.decl)))
+	  && !DECL_VIRTUAL_P (node->symbol.decl)
 	  /* During early inlining some unanalyzed cgraph nodes might be in the
 	     callgraph and they might reffer the function in question.  */
 	  && !cgraph_new_nodes);
Index: src/gcc/ipa.c
===================================================================
--- src.orig/gcc/ipa.c
+++ src/gcc/ipa.c
@@ -241,8 +241,7 @@  symtab_remove_unreachable_nodes (bool be
 	&& (!cgraph_can_remove_if_no_direct_calls_and_refs_p (node)
 	    /* Keep around virtual functions for possible devirtualization.  */
 	    || (before_inlining_p
-		&& DECL_VIRTUAL_P (node->symbol.decl)
-		&& (DECL_COMDAT (node->symbol.decl) || DECL_EXTERNAL (node->symbol.decl)))))
+		&& DECL_VIRTUAL_P (node->symbol.decl))))
       {
         gcc_assert (!node->global.inlined_to);
 	pointer_set_insert (reachable, node);
Index: src/gcc/testsuite/g++.dg/ipa/pr55264.C
===================================================================
--- /dev/null
+++ src/gcc/testsuite/g++.dg/ipa/pr55264.C
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-early-inlining -fno-weak"  } */
+
+struct S
+{
+  S();
+  virtual inline void foo ()
+  {
+    foo();
+  }
+};
+
+void
+B ()
+{
+  S().foo ();
+}
Index: src/gcc/cgraph.h
===================================================================
--- src.orig/gcc/cgraph.h
+++ src/gcc/cgraph.h
@@ -1164,6 +1164,7 @@  cgraph_only_called_directly_or_aliased_p
   gcc_assert (!node->global.inlined_to);
   return (!node->symbol.force_output && !node->symbol.address_taken
 	  && !node->symbol.used_from_other_partition
+	  && !DECL_VIRTUAL_P (node->symbol.decl)
 	  && !DECL_STATIC_CONSTRUCTOR (node->symbol.decl)
 	  && !DECL_STATIC_DESTRUCTOR (node->symbol.decl)
 	  && !node->symbol.externally_visible);
Index: src/gcc/cgraphclones.c
===================================================================
--- src.orig/gcc/cgraphclones.c
+++ src/gcc/cgraphclones.c
@@ -319,6 +319,7 @@  cgraph_create_virtual_clone (struct cgra
   TREE_PUBLIC (new_node->symbol.decl) = 0;
   DECL_COMDAT (new_node->symbol.decl) = 0;
   DECL_WEAK (new_node->symbol.decl) = 0;
+  DECL_VIRTUAL_P (new_node->symbol.decl) = 0;
   DECL_STATIC_CONSTRUCTOR (new_node->symbol.decl) = 0;
   DECL_STATIC_DESTRUCTOR (new_node->symbol.decl) = 0;
   new_node->clone.tree_map = tree_map;