Avoid trashing of polymorphic call cache during inlining

Message ID 20191104194459.d77nbrsmvtsizwqz@kam.mff.cuni.cz
State New
Headers show
Series
  • Avoid trashing of polymorphic call cache during inlining
Related show

Commit Message

Jan Hubicka Nov. 4, 2019, 7:44 p.m.
Hi,
I am not really pround of this implementation (and will think of better
interface), but this patch saves about 10% of WPA time by avoiding
unnecesary invalidations of the polymorphic call target hash during
inlining.

ipa-devirt register node removal hook to invalidate cache when one of
functions in it gets removed.  Now inliner often decides to inline into
a thunk. In order to get costs right it turns the thunk into a gimple
functions and re-inserts it into the summaries (so the summaries gets
computed for the actual thunk body). 

Bootstrapped/regtested x86_64-linux, comitted.

	* ipa-inline-transform.c: Include ipa-utils.h
	(inline_call): Set thunk_expansion flag.
	* ipa-utils.h (thunk_expansion): Declare.
	* ipa-devirt.c (thunk_expansion): New global var.
	(devirt_node_removal_hook): Do not invalidate cache while
	doing thunk expansion.

Comments

Martin Jambor Nov. 14, 2019, 1:41 p.m. | #1
Hi,

On Mon, Nov 04 2019, Jan Hubicka wrote:
> Hi,

> I am not really pround of this implementation (and will think of better

> interface), but this patch saves about 10% of WPA time by avoiding

> unnecesary invalidations of the polymorphic call target hash during

> inlining.

>

> ipa-devirt register node removal hook to invalidate cache when one of

> functions in it gets removed.  Now inliner often decides to inline into

> a thunk. In order to get costs right it turns the thunk into a gimple

> functions and re-inserts it into the summaries (so the summaries gets

> computed for the actual thunk body). 

>

> Bootstrapped/regtested x86_64-linux, comitted.

>

> 	* ipa-inline-transform.c: Include ipa-utils.h

> 	(inline_call): Set thunk_expansion flag.

> 	* ipa-utils.h (thunk_expansion): Declare.

> 	* ipa-devirt.c (thunk_expansion): New global var.

> 	(devirt_node_removal_hook): Do not invalidate cache while

> 	doing thunk expansion.


...

> Index: ipa-utils.h

> ===================================================================

> --- ipa-utils.h	(revision 277780)

> +++ ipa-utils.h	(working copy)

> @@ -47,6 +47,9 @@ void ipa_merge_profiles (struct cgraph_n

>  			 struct cgraph_node *src, bool preserve_body = false);

>  bool recursive_call_p (tree, tree);

>  

> +/* In ipa-prop.c  */

> +void ipa_remove_useless_jump_functions ();

> +


This is probably an unintended change?  Can I remove it?

Martin
Jan Hubicka Nov. 14, 2019, 1:44 p.m. | #2
> Hi,

> 

> On Mon, Nov 04 2019, Jan Hubicka wrote:

> > Hi,

> > I am not really pround of this implementation (and will think of better

> > interface), but this patch saves about 10% of WPA time by avoiding

> > unnecesary invalidations of the polymorphic call target hash during

> > inlining.

> >

> > ipa-devirt register node removal hook to invalidate cache when one of

> > functions in it gets removed.  Now inliner often decides to inline into

> > a thunk. In order to get costs right it turns the thunk into a gimple

> > functions and re-inserts it into the summaries (so the summaries gets

> > computed for the actual thunk body). 

> >

> > Bootstrapped/regtested x86_64-linux, comitted.

> >

> > 	* ipa-inline-transform.c: Include ipa-utils.h

> > 	(inline_call): Set thunk_expansion flag.

> > 	* ipa-utils.h (thunk_expansion): Declare.

> > 	* ipa-devirt.c (thunk_expansion): New global var.

> > 	(devirt_node_removal_hook): Do not invalidate cache while

> > 	doing thunk expansion.

> 

> ...

> 

> > Index: ipa-utils.h

> > ===================================================================

> > --- ipa-utils.h	(revision 277780)

> > +++ ipa-utils.h	(working copy)

> > @@ -47,6 +47,9 @@ void ipa_merge_profiles (struct cgraph_n

> >  			 struct cgraph_node *src, bool preserve_body = false);

> >  bool recursive_call_p (tree, tree);

> >  

> > +/* In ipa-prop.c  */

> > +void ipa_remove_useless_jump_functions ();

> > +

> 

> This is probably an unintended change?  Can I remove it?


Indeed, it is unrelated change.
Thanks for noticing it!
Honza
> 

> Martin

>
Martin Jambor Nov. 15, 2019, 4:20 p.m. | #3
On Thu, Nov 14 2019, Jan Hubicka wrote:
>> On Mon, Nov 04 2019, Jan Hubicka wrote:

>> > Index: ipa-utils.h

>> > ===================================================================

>> > --- ipa-utils.h	(revision 277780)

>> > +++ ipa-utils.h	(working copy)

>> > @@ -47,6 +47,9 @@ void ipa_merge_profiles (struct cgraph_n

>> >  			 struct cgraph_node *src, bool preserve_body = false);

>> >  bool recursive_call_p (tree, tree);

>> >  

>> > +/* In ipa-prop.c  */

>> > +void ipa_remove_useless_jump_functions ();

>> > +

>> 

>> This is probably an unintended change?  Can I remove it?

>

> Indeed, it is unrelated change.

> Thanks for noticing it!

> Honza


OK, I have committed the following (after adding to a round of bootstrap
and testing).

Thanks,

Martin


2019-11-15  Martin Jambor  <mjambor@suse.cz>

	* ipa-utils.h (ipa_remove_useless_jump_functions): Remove stray
	declaration.
---
 gcc/ipa-utils.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
index 947307a3d66..60c52e0fa53 100644
--- a/gcc/ipa-utils.h
+++ b/gcc/ipa-utils.h
@@ -47,9 +47,6 @@ void ipa_merge_profiles (struct cgraph_node *dst,
 			 struct cgraph_node *src, bool preserve_body = false);
 bool recursive_call_p (tree, tree);
 
-/* In ipa-prop.c  */
-void ipa_remove_useless_jump_functions ();
-
 /* In ipa-profile.c  */
 bool ipa_propagate_frequency (struct cgraph_node *node);
 
-- 
2.23.0

Patch

Index: ipa-inline-transform.c
===================================================================
--- ipa-inline-transform.c	(revision 277780)
+++ ipa-inline-transform.c	(working copy)
@@ -47,6 +47,7 @@  along with GCC; see the file COPYING3.
 #include "function.h"
 #include "cfg.h"
 #include "basic-block.h"
+#include "ipa-utils.h"
 
 int ncalls_inlined;
 int nfunctions_inlined;
@@ -352,6 +353,7 @@  inline_call (struct cgraph_edge *e, bool
   if (to->thunk.thunk_p)
     {
       struct cgraph_node *target = to->callees->callee;
+      thunk_expansion = true;
       symtab->call_cgraph_removal_hooks (to);
       if (in_lto_p)
 	to->get_untransformed_body ();
@@ -360,6 +362,7 @@  inline_call (struct cgraph_edge *e, bool
       for (e = to->callees; e && e->callee != target; e = e->next_callee)
 	;
       symtab->call_cgraph_insertion_hooks (to);
+      thunk_expansion = false;
       gcc_assert (e);
     }
 
Index: ipa-utils.h
===================================================================
--- ipa-utils.h	(revision 277780)
+++ ipa-utils.h	(working copy)
@@ -47,6 +47,9 @@  void ipa_merge_profiles (struct cgraph_n
 			 struct cgraph_node *src, bool preserve_body = false);
 bool recursive_call_p (tree, tree);
 
+/* In ipa-prop.c  */
+void ipa_remove_useless_jump_functions ();
+
 /* In ipa-profile.c  */
 bool ipa_propagate_frequency (struct cgraph_node *node);
 
@@ -54,6 +57,7 @@  bool ipa_propagate_frequency (struct cgr
 
 struct odr_type_d;
 typedef odr_type_d *odr_type;
+extern bool thunk_expansion;
 void build_type_inheritance_graph (void);
 void rebuild_type_inheritance_graph (void);
 void update_type_inheritance_graph (void);
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c	(revision 277780)
+++ ipa-devirt.c	(working copy)
@@ -172,6 +172,11 @@  struct default_hash_traits <type_pair>
     }
 };
 
+/* HACK alert: this is used to communicate with ipa-inline-transform that
+   thunk is being expanded and there is no need to clear the polymorphic
+   call target cache.  */
+bool thunk_expansion;
+
 static bool odr_types_equivalent_p (tree, tree, bool, bool *,
 				    hash_set<type_pair> *,
 				    location_t, location_t);
@@ -2747,6 +2752,7 @@  static void
 devirt_node_removal_hook (struct cgraph_node *n, void *d ATTRIBUTE_UNUSED)
 {
   if (cached_polymorphic_call_targets
+      && !thunk_expansion
       && cached_polymorphic_call_targets->contains (n))
     free_polymorphic_call_targets_hash ();
 }