Avoid trashing of polymorphic call cache during inlining
diff mbox series

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. UTC
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. UTC | #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. UTC | #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. UTC | #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);

Patch
diff mbox series

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 ();
 }