diff mbox series

Construct ipa_reduced_postorder always for overwritable (PR ipa/89009).

Message ID 376f68fe-91e9-20dc-894d-eac4e085b7dc@suse.cz
State New
Headers show
Series Construct ipa_reduced_postorder always for overwritable (PR ipa/89009). | expand

Commit Message

Martin Liška Feb. 11, 2019, 7:59 a.m. UTC
Hi.

IPA pure const should always construct ipa_reduced_postorder with
possibility to cross AVAIL_INTERPOSABLE boundary. The pass itself
can then properly stop propagation on these symbols.

The patch is pre-approved by Honza.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Thanks,
Martin

gcc/ChangeLog:

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

	PR ipa/89009
	* ipa-cp.c (build_toporder_info): Remove usage of a param.
	* ipa-inline.c (inline_small_functions): Likewise.
	* ipa-pure-const.c (propagate_pure_const): Likewise.
	(propagate_nothrow): Likewise.
	* ipa-reference.c (propagate): Likewise.
	* ipa-utils.c (struct searchc_env): Remove unused field.
	(searchc): Always search across AVAIL_INTERPOSABLE.
	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
	the only called IPA pure const can properly not propagate
	across interposable boundary.
	* ipa-utils.h (ipa_reduced_postorder): Remove param.
---
 gcc/ipa-cp.c         | 2 +-
 gcc/ipa-inline.c     | 2 +-
 gcc/ipa-pure-const.c | 4 ++--
 gcc/ipa-reference.c  | 2 +-
 gcc/ipa-utils.c      | 9 +++------
 gcc/ipa-utils.h      | 2 +-
 6 files changed, 9 insertions(+), 12 deletions(-)

Comments

Martin Liška Feb. 11, 2019, 8:12 a.m. UTC | #1
Updated version of the patch where I added a test-case.
I'm going to install this version.

Martin
From ffe0985846a8827fce4e47f090804e664059b650 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 8 Feb 2019 13:04:11 +0100
Subject: [PATCH] Construct ipa_reduced_postorder always for overwritable (PR
 ipa/89009).

gcc/ChangeLog:

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

	PR ipa/89009
	* ipa-cp.c (build_toporder_info): Remove usage of a param.
	* ipa-inline.c (inline_small_functions): Likewise.
	* ipa-pure-const.c (propagate_pure_const): Likewise.
	(propagate_nothrow): Likewise.
	* ipa-reference.c (propagate): Likewise.
	* ipa-utils.c (struct searchc_env): Remove unused field.
	(searchc): Always search across AVAIL_INTERPOSABLE.
	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
	the only called IPA pure const can properly not propagate
	across interposable boundary.
	* ipa-utils.h (ipa_reduced_postorder): Remove param.

gcc/testsuite/ChangeLog:

2019-02-11  Martin Liska  <mliska@suse.cz>

	PR ipa/89009
	* g++.dg/ipa/pr89009.C: New test.
---
 gcc/ipa-cp.c                       |  2 +-
 gcc/ipa-inline.c                   |  2 +-
 gcc/ipa-pure-const.c               |  4 ++--
 gcc/ipa-reference.c                |  2 +-
 gcc/ipa-utils.c                    |  9 +++------
 gcc/ipa-utils.h                    |  2 +-
 gcc/testsuite/g++.dg/ipa/pr89009.C | 12 ++++++++++++
 7 files changed, 21 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr89009.C

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 3489ed59ce2..442d5c63eff 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -815,7 +815,7 @@ build_toporder_info (struct ipa_topo_info *topo)
   topo->stack = XCNEWVEC (struct cgraph_node *, symtab->cgraph_count);
 
   gcc_checking_assert (topo->stack_top == 0);
-  topo->nnodes = ipa_reduced_postorder (topo->order, true, true, NULL);
+  topo->nnodes = ipa_reduced_postorder (topo->order, true, NULL);
 }
 
 /* Free information about strongly connected components and the arrays in
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 4ddbfdf772c..360c3de3289 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -1778,7 +1778,7 @@ inline_small_functions (void)
      metrics.  */
 
   max_count = profile_count::uninitialized ();
-  ipa_reduced_postorder (order, true, true, NULL);
+  ipa_reduced_postorder (order, true, NULL);
   free (order);
 
   FOR_EACH_DEFINED_FUNCTION (node)
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 8227eed29bc..a8a3956d2d5 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -1422,7 +1422,7 @@ propagate_pure_const (void)
   bool remove_p = false;
   bool has_cdtor;
 
-  order_pos = ipa_reduced_postorder (order, true, false,
+  order_pos = ipa_reduced_postorder (order, true,
 				     ignore_edge_for_pure_const);
   if (dump_file)
     {
@@ -1751,7 +1751,7 @@ propagate_nothrow (void)
   int i;
   struct ipa_dfs_info * w_info;
 
-  order_pos = ipa_reduced_postorder (order, true, false,
+  order_pos = ipa_reduced_postorder (order, true,
 				     ignore_edge_for_nothrow);
   if (dump_file)
     {
diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 8ad12d30bb2..d1759a374bc 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -712,7 +712,7 @@ propagate (void)
      the global information.  All the nodes within a cycle will have
      the same info so we collapse cycles first.  Then we can do the
      propagation in one pass from the leaves to the roots.  */
-  order_pos = ipa_reduced_postorder (order, true, true, ignore_edge_p);
+  order_pos = ipa_reduced_postorder (order, true, ignore_edge_p);
   if (dump_file)
     ipa_print_order (dump_file, "reduced", order, order_pos);
 
diff --git a/gcc/ipa-utils.c b/gcc/ipa-utils.c
index aebcb8ea91d..79b250c3943 100644
--- a/gcc/ipa-utils.c
+++ b/gcc/ipa-utils.c
@@ -63,7 +63,6 @@ struct searchc_env {
   int order_pos;
   splay_tree nodes_marked_new;
   bool reduce;
-  bool allow_overwritable;
   int count;
 };
 
@@ -105,7 +104,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
 
       if (w->aux
 	  && (avail > AVAIL_INTERPOSABLE
-	      || (env->allow_overwritable && avail == AVAIL_INTERPOSABLE)))
+	      || avail == AVAIL_INTERPOSABLE))
 	{
 	  w_info = (struct ipa_dfs_info *) w->aux;
 	  if (w_info->new_node)
@@ -162,7 +161,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
 
 int
 ipa_reduced_postorder (struct cgraph_node **order,
-		       bool reduce, bool allow_overwritable,
+		       bool reduce,
 		       bool (*ignore_edge) (struct cgraph_edge *))
 {
   struct cgraph_node *node;
@@ -175,15 +174,13 @@ ipa_reduced_postorder (struct cgraph_node **order,
   env.nodes_marked_new = splay_tree_new (splay_tree_compare_ints, 0, 0);
   env.count = 1;
   env.reduce = reduce;
-  env.allow_overwritable = allow_overwritable;
 
   FOR_EACH_DEFINED_FUNCTION (node)
     {
       enum availability avail = node->get_availability ();
 
       if (avail > AVAIL_INTERPOSABLE
-	  || (allow_overwritable
-	      && (avail == AVAIL_INTERPOSABLE)))
+	  || avail == AVAIL_INTERPOSABLE)
 	{
 	  /* Reuse the info if it is already there.  */
 	  struct ipa_dfs_info *info = (struct ipa_dfs_info *) node->aux;
diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
index 618d74f78ee..b70e8c57108 100644
--- a/gcc/ipa-utils.h
+++ b/gcc/ipa-utils.h
@@ -36,7 +36,7 @@ struct ipa_dfs_info {
 
 /* In ipa-utils.c  */
 void ipa_print_order (FILE*, const char *, struct cgraph_node**, int);
-int ipa_reduced_postorder (struct cgraph_node **, bool, bool,
+int ipa_reduced_postorder (struct cgraph_node **, bool,
 			  bool (*ignore_edge) (struct cgraph_edge *));
 void ipa_free_postorder_info (void);
 vec<cgraph_node *> ipa_get_nodes_in_cycle (struct cgraph_node *);
diff --git a/gcc/testsuite/g++.dg/ipa/pr89009.C b/gcc/testsuite/g++.dg/ipa/pr89009.C
new file mode 100644
index 00000000000..6b4fc65a641
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr89009.C
@@ -0,0 +1,12 @@
+/* PR ipa/89009 */
+/* { dg-do run } */
+/* { dg-options "-fvisibility=hidden -fpic -O2 -fno-inline" } */
+
+#pragma GCC visibility push(default)
+void foo1() { __builtin_printf ("foo\n"); }
+#pragma GCC visibility pop
+void foo2() { __builtin_printf ("foo\n"); }
+
+int main() { foo2(); return 0; }
+
+/* { dg-output "foo" } */
Jan Hubicka Feb. 11, 2019, 8:52 a.m. UTC | #2
> Hi.
> 
> IPA pure const should always construct ipa_reduced_postorder with
> possibility to cross AVAIL_INTERPOSABLE boundary. The pass itself
> can then properly stop propagation on these symbols.
> 
> The patch is pre-approved by Honza.
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2019-02-08  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/89009
> 	* ipa-cp.c (build_toporder_info): Remove usage of a param.
> 	* ipa-inline.c (inline_small_functions): Likewise.
> 	* ipa-pure-const.c (propagate_pure_const): Likewise.
> 	(propagate_nothrow): Likewise.
> 	* ipa-reference.c (propagate): Likewise.
> 	* ipa-utils.c (struct searchc_env): Remove unused field.
> 	(searchc): Always search across AVAIL_INTERPOSABLE.
> 	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
> 	the only called IPA pure const can properly not propagate
> 	across interposable boundary.
> 	* ipa-utils.h (ipa_reduced_postorder): Remove param.
> @@ -105,7 +104,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
>  
>        if (w->aux
>  	  && (avail > AVAIL_INTERPOSABLE
> -	      || (env->allow_overwritable && avail == AVAIL_INTERPOSABLE)))
> +	      || avail == AVAIL_INTERPOSABLE))
>  	{
>  	  w_info = (struct ipa_dfs_info *) w->aux;
>  	  if (w_info->new_node)

This optimizes >= AVAIL_INTERPOSABLE then, but for ipa-cp, ipa-inline,
ipa-pure-const we do not want interposable calls to close SCC regions
(because we can not propagate here). ipa-reference is more subtle -
interposable calls close SCC regions IFF they are declared leaf.

Can you, please, update ignore_edge_p of indiviual optimization passes
this way? (I.e. for all but ipa-reference add aval <= AVAIL_INTERPOSABLE
check for the availability of the target function and for ipa-reference
test also leaf attribute?). 

In all those cases we also want to check that given optimization pass is
enabled for both caller and callee.  In other cases we do not want to
consider them part of SCCs as well.  It may make sense to do this as one
change or I can do that incrementally.

Thanks,
Honza
Martin Jambor Feb. 11, 2019, 1:34 p.m. UTC | #3
Hi,

On Mon, Feb 11 2019, Jan Hubicka wrote:
>> Hi.
>> 
>> IPA pure const should always construct ipa_reduced_postorder with
>> possibility to cross AVAIL_INTERPOSABLE boundary. The pass itself
>> can then properly stop propagation on these symbols.
>> 
>> The patch is pre-approved by Honza.
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>> 
>> Thanks,
>> Martin
>> 
>> gcc/ChangeLog:
>> 
>> 2019-02-08  Martin Liska  <mliska@suse.cz>
>> 
>> 	PR ipa/89009
>> 	* ipa-cp.c (build_toporder_info): Remove usage of a param.
>> 	* ipa-inline.c (inline_small_functions): Likewise.
>> 	* ipa-pure-const.c (propagate_pure_const): Likewise.
>> 	(propagate_nothrow): Likewise.
>> 	* ipa-reference.c (propagate): Likewise.
>> 	* ipa-utils.c (struct searchc_env): Remove unused field.
>> 	(searchc): Always search across AVAIL_INTERPOSABLE.
>> 	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
>> 	the only called IPA pure const can properly not propagate
>> 	across interposable boundary.
>> 	* ipa-utils.h (ipa_reduced_postorder): Remove param.
>> @@ -105,7 +104,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
>>  
>>        if (w->aux
>>  	  && (avail > AVAIL_INTERPOSABLE
>> -	      || (env->allow_overwritable && avail == AVAIL_INTERPOSABLE)))
>> +	      || avail == AVAIL_INTERPOSABLE))

This looks a tad too mechanical, surely

   (avail > AVAIL_INTERPOSABLE || avail == AVAIL_INTERPOSABLE)

can be simplified into just one comparison :-)

>>  	{
>>  	  w_info = (struct ipa_dfs_info *) w->aux;
>>  	  if (w_info->new_node)
>
> This optimizes >= AVAIL_INTERPOSABLE then, but for ipa-cp, ipa-inline,
> ipa-pure-const we do not want interposable calls to close SCC regions
> (because we can not propagate here). ipa-reference is more subtle -
> interposable calls close SCC regions IFF they are declared leaf.
>
> Can you, please, update ignore_edge_p of indiviual optimization passes
> this way? (I.e. for all but ipa-reference add aval <= AVAIL_INTERPOSABLE
> check for the availability of the target function and for ipa-reference
> test also leaf attribute?). 

At least IPA-CP does not have any ignore_edge_p... does it really need
any?  Such node can incur an extra iteration of the propagator over an
entire SCC, that is true, but is it worth adding a callback?  Are SCCs
with an interposable node common?

>
> In all those cases we also want to check that given optimization pass is
> enabled for both caller and callee.  In other cases we do not want to
> consider them part of SCCs as well.  It may make sense to do this as one
> change or I can do that incrementally.

Likewise.

Martin
Jan Hubicka Feb. 11, 2019, 1:49 p.m. UTC | #4
> Hi,
> 
> On Mon, Feb 11 2019, Jan Hubicka wrote:
> >> Hi.
> >> 
> >> IPA pure const should always construct ipa_reduced_postorder with
> >> possibility to cross AVAIL_INTERPOSABLE boundary. The pass itself
> >> can then properly stop propagation on these symbols.
> >> 
> >> The patch is pre-approved by Honza.
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >> 
> >> Thanks,
> >> Martin
> >> 
> >> gcc/ChangeLog:
> >> 
> >> 2019-02-08  Martin Liska  <mliska@suse.cz>
> >> 
> >> 	PR ipa/89009
> >> 	* ipa-cp.c (build_toporder_info): Remove usage of a param.
> >> 	* ipa-inline.c (inline_small_functions): Likewise.
> >> 	* ipa-pure-const.c (propagate_pure_const): Likewise.
> >> 	(propagate_nothrow): Likewise.
> >> 	* ipa-reference.c (propagate): Likewise.
> >> 	* ipa-utils.c (struct searchc_env): Remove unused field.
> >> 	(searchc): Always search across AVAIL_INTERPOSABLE.
> >> 	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
> >> 	the only called IPA pure const can properly not propagate
> >> 	across interposable boundary.
> >> 	* ipa-utils.h (ipa_reduced_postorder): Remove param.
> >> @@ -105,7 +104,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
> >>  
> >>        if (w->aux
> >>  	  && (avail > AVAIL_INTERPOSABLE
> >> -	      || (env->allow_overwritable && avail == AVAIL_INTERPOSABLE)))
> >> +	      || avail == AVAIL_INTERPOSABLE))
> 
> This looks a tad too mechanical, surely
> 
>    (avail > AVAIL_INTERPOSABLE || avail == AVAIL_INTERPOSABLE)
> 
> can be simplified into just one comparison :-)
> 
> >>  	{
> >>  	  w_info = (struct ipa_dfs_info *) w->aux;
> >>  	  if (w_info->new_node)
> >
> > This optimizes >= AVAIL_INTERPOSABLE then, but for ipa-cp, ipa-inline,
> > ipa-pure-const we do not want interposable calls to close SCC regions
> > (because we can not propagate here). ipa-reference is more subtle -
> > interposable calls close SCC regions IFF they are declared leaf.
> >
> > Can you, please, update ignore_edge_p of indiviual optimization passes
> > this way? (I.e. for all but ipa-reference add aval <= AVAIL_INTERPOSABLE
> > check for the availability of the target function and for ipa-reference
> > test also leaf attribute?). 
> 
> At least IPA-CP does not have any ignore_edge_p... does it really need
> any?  Such node can incur an extra iteration of the propagator over an

You probably want to ignore edges with no useful jump functions to get
finer SCC structure as well.

> entire SCC, that is true, but is it worth adding a callback?  Are SCCs
> with an interposable node common?

In a PIC library without visibility attributes it would be all of them.
Yep, this is mostly about quality of implementation - with SCCs bigger
than necessary code will work, just produce worse code.

Honza
> 
> >
> > In all those cases we also want to check that given optimization pass is
> > enabled for both caller and callee.  In other cases we do not want to
> > consider them part of SCCs as well.  It may make sense to do this as one
> > change or I can do that incrementally.
> 
> Likewise.
> 
> Martin
Martin Liška Feb. 13, 2019, 6 a.m. UTC | #5
Hi.

This is patch candidate I created and tested. It's not adding
filtering based on opt_for_fn which I would defer to the next
stage1.

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

Ready to be installed?
Thanks,
Martin
From d036f75a880bc91f67a5473767b35ba2f8a4ffe3 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 11 Feb 2019 16:47:06 +0100
Subject: [PATCH] Reduce SCCs in IPA postorder.

gcc/ChangeLog:

2019-02-13  Martin Liska  <mliska@suse.cz>

	* ipa-cp.c (build_toporder_info): Use
	ignore_edge_if_not_available as edge filter.
	* ipa-inline.c (inline_small_functions): Likewise.
	* ipa-pure-const.c (ignore_edge_for_pure_const):
	Move to ipa-utils.h and rename to ignore_edge_if_not_available.
	(propagate_pure_const): Use ignore_edge_if_not_available
	as edge filter.
	* ipa-reference.c (ignore_edge_p): Make SCCs more fine
	based on availability and ECF_LEAF attribute.
	* ipa-utils.c (searchc): Refactor code.
	* ipa-utils.h (ignore_edge_if_not_available): New.
---
 gcc/ipa-cp.c         |  3 ++-
 gcc/ipa-inline.c     |  2 +-
 gcc/ipa-pure-const.c | 13 +------------
 gcc/ipa-reference.c  | 13 ++++++++++---
 gcc/ipa-utils.c      |  3 +--
 gcc/ipa-utils.h      | 10 ++++++++++
 6 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 442d5c63eff..2253b0cef63 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -815,7 +815,8 @@ build_toporder_info (struct ipa_topo_info *topo)
   topo->stack = XCNEWVEC (struct cgraph_node *, symtab->cgraph_count);
 
   gcc_checking_assert (topo->stack_top == 0);
-  topo->nnodes = ipa_reduced_postorder (topo->order, true, NULL);
+  topo->nnodes = ipa_reduced_postorder (topo->order, true,
+					ignore_edge_if_not_available);
 }
 
 /* Free information about strongly connected components and the arrays in
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 360c3de3289..c7e68a73706 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -1778,7 +1778,7 @@ inline_small_functions (void)
      metrics.  */
 
   max_count = profile_count::uninitialized ();
-  ipa_reduced_postorder (order, true, NULL);
+  ipa_reduced_postorder (order, true, ignore_edge_if_not_available);
   free (order);
 
   FOR_EACH_DEFINED_FUNCTION (node)
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index a8a3956d2d5..e61d279289e 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -1395,17 +1395,6 @@ cdtor_p (cgraph_node *n, void *)
   return false;
 }
 
-/* We only propagate across edges with non-interposable callee.  */
-
-static bool
-ignore_edge_for_pure_const (struct cgraph_edge *e)
-{
-  enum availability avail;
-  e->callee->function_or_virtual_thunk_symbol (&avail, e->caller);
-  return (avail <= AVAIL_INTERPOSABLE);
-}
-
-
 /* Produce transitive closure over the callgraph and compute pure/const
    attributes.  */
 
@@ -1423,7 +1412,7 @@ propagate_pure_const (void)
   bool has_cdtor;
 
   order_pos = ipa_reduced_postorder (order, true,
-				     ignore_edge_for_pure_const);
+				     ignore_edge_if_not_available);
   if (dump_file)
     {
       cgraph_node::dump_cgraph (dump_file);
diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index d1759a374bc..16cc4cf44f9 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -677,14 +677,21 @@ get_read_write_all_from_node (struct cgraph_node *node,
       }
 }
 
-/* Skip edges from and to nodes without ipa_reference enables.  This leave
-   them out of strongy connected coponents and makes them easyto skip in the
+/* Skip edges from and to nodes without ipa_reference enabled.
+   Ignore not available symbols.  This leave
+   them out of strongly connected components and makes them easy to skip in the
    propagation loop bellow.  */
 
 static bool
 ignore_edge_p (cgraph_edge *e)
 {
-  return (!opt_for_fn (e->caller->decl, flag_ipa_reference)
+  enum availability avail;
+  e->callee->function_or_virtual_thunk_symbol (&avail, e->caller);
+
+  return (avail < AVAIL_INTERPOSABLE
+	  || (avail == AVAIL_INTERPOSABLE
+	      && !(flags_from_decl_or_type (e->callee->decl) & ECF_LEAF))
+	  || !opt_for_fn (e->caller->decl, flag_ipa_reference)
           || !opt_for_fn (e->callee->function_symbol ()->decl,
 			  flag_ipa_reference));
 }
diff --git a/gcc/ipa-utils.c b/gcc/ipa-utils.c
index 79b250c3943..25c2e2cf789 100644
--- a/gcc/ipa-utils.c
+++ b/gcc/ipa-utils.c
@@ -103,8 +103,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
         continue;
 
       if (w->aux
-	  && (avail > AVAIL_INTERPOSABLE
-	      || avail == AVAIL_INTERPOSABLE))
+	  && (avail >= AVAIL_INTERPOSABLE))
 	{
 	  w_info = (struct ipa_dfs_info *) w->aux;
 	  if (w_info->new_node)
diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
index b70e8c57108..aad08148348 100644
--- a/gcc/ipa-utils.h
+++ b/gcc/ipa-utils.h
@@ -262,6 +262,16 @@ odr_type_p (const_tree t)
   return false;
 }
 
+/* We only propagate across edges with non-interposable callee.  */
+
+inline bool
+ignore_edge_if_not_available (struct cgraph_edge *e)
+{
+  enum availability avail;
+  e->callee->function_or_virtual_thunk_symbol (&avail, e->caller);
+  return (avail <= AVAIL_INTERPOSABLE);
+}
+
 #endif  /* GCC_IPA_UTILS_H  */
Jan Hubicka Feb. 14, 2019, 10:19 a.m. UTC | #6
> Hi.
> 
> This is patch candidate I created and tested. It's not adding
> filtering based on opt_for_fn which I would defer to the next
> stage1.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin

> From d036f75a880bc91f67a5473767b35ba2f8a4ffe3 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Mon, 11 Feb 2019 16:47:06 +0100
> Subject: [PATCH] Reduce SCCs in IPA postorder.
> 
> gcc/ChangeLog:
> 
> 2019-02-13  Martin Liska  <mliska@suse.cz>
> 
> 	* ipa-cp.c (build_toporder_info): Use
> 	ignore_edge_if_not_available as edge filter.
> 	* ipa-inline.c (inline_small_functions): Likewise.
> 	* ipa-pure-const.c (ignore_edge_for_pure_const):
> 	Move to ipa-utils.h and rename to ignore_edge_if_not_available.
> 	(propagate_pure_const): Use ignore_edge_if_not_available
> 	as edge filter.
> 	* ipa-reference.c (ignore_edge_p): Make SCCs more fine
> 	based on availability and ECF_LEAF attribute.
> 	* ipa-utils.c (searchc): Refactor code.
> 	* ipa-utils.h (ignore_edge_if_not_available): New.

OK, I think it is safe to wait for stage1 - it is bit fragile to
propagate across different graph then postorder is computed (as
manifested by the bug) but it should be safe if SCCs are simply bigger
then they should be.

Next stage1 we should also teach the callback to ignore edges of calls
that are not being optimized.

Honza
> ---
>  gcc/ipa-cp.c         |  3 ++-
>  gcc/ipa-inline.c     |  2 +-
>  gcc/ipa-pure-const.c | 13 +------------
>  gcc/ipa-reference.c  | 13 ++++++++++---
>  gcc/ipa-utils.c      |  3 +--
>  gcc/ipa-utils.h      | 10 ++++++++++
>  6 files changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 442d5c63eff..2253b0cef63 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -815,7 +815,8 @@ build_toporder_info (struct ipa_topo_info *topo)
>    topo->stack = XCNEWVEC (struct cgraph_node *, symtab->cgraph_count);
>  
>    gcc_checking_assert (topo->stack_top == 0);
> -  topo->nnodes = ipa_reduced_postorder (topo->order, true, NULL);
> +  topo->nnodes = ipa_reduced_postorder (topo->order, true,
> +					ignore_edge_if_not_available);
>  }
>  
>  /* Free information about strongly connected components and the arrays in
> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> index 360c3de3289..c7e68a73706 100644
> --- a/gcc/ipa-inline.c
> +++ b/gcc/ipa-inline.c
> @@ -1778,7 +1778,7 @@ inline_small_functions (void)
>       metrics.  */
>  
>    max_count = profile_count::uninitialized ();
> -  ipa_reduced_postorder (order, true, NULL);
> +  ipa_reduced_postorder (order, true, ignore_edge_if_not_available);
>    free (order);
>  
>    FOR_EACH_DEFINED_FUNCTION (node)
> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
> index a8a3956d2d5..e61d279289e 100644
> --- a/gcc/ipa-pure-const.c
> +++ b/gcc/ipa-pure-const.c
> @@ -1395,17 +1395,6 @@ cdtor_p (cgraph_node *n, void *)
>    return false;
>  }
>  
> -/* We only propagate across edges with non-interposable callee.  */
> -
> -static bool
> -ignore_edge_for_pure_const (struct cgraph_edge *e)
> -{
> -  enum availability avail;
> -  e->callee->function_or_virtual_thunk_symbol (&avail, e->caller);
> -  return (avail <= AVAIL_INTERPOSABLE);
> -}
> -
> -
>  /* Produce transitive closure over the callgraph and compute pure/const
>     attributes.  */
>  
> @@ -1423,7 +1412,7 @@ propagate_pure_const (void)
>    bool has_cdtor;
>  
>    order_pos = ipa_reduced_postorder (order, true,
> -				     ignore_edge_for_pure_const);
> +				     ignore_edge_if_not_available);
>    if (dump_file)
>      {
>        cgraph_node::dump_cgraph (dump_file);
> diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
> index d1759a374bc..16cc4cf44f9 100644
> --- a/gcc/ipa-reference.c
> +++ b/gcc/ipa-reference.c
> @@ -677,14 +677,21 @@ get_read_write_all_from_node (struct cgraph_node *node,
>        }
>  }
>  
> -/* Skip edges from and to nodes without ipa_reference enables.  This leave
> -   them out of strongy connected coponents and makes them easyto skip in the
> +/* Skip edges from and to nodes without ipa_reference enabled.
> +   Ignore not available symbols.  This leave
> +   them out of strongly connected components and makes them easy to skip in the
>     propagation loop bellow.  */
>  
>  static bool
>  ignore_edge_p (cgraph_edge *e)
>  {
> -  return (!opt_for_fn (e->caller->decl, flag_ipa_reference)
> +  enum availability avail;
> +  e->callee->function_or_virtual_thunk_symbol (&avail, e->caller);
> +
> +  return (avail < AVAIL_INTERPOSABLE
> +	  || (avail == AVAIL_INTERPOSABLE
> +	      && !(flags_from_decl_or_type (e->callee->decl) & ECF_LEAF))
> +	  || !opt_for_fn (e->caller->decl, flag_ipa_reference)
>            || !opt_for_fn (e->callee->function_symbol ()->decl,
>  			  flag_ipa_reference));
>  }
> diff --git a/gcc/ipa-utils.c b/gcc/ipa-utils.c
> index 79b250c3943..25c2e2cf789 100644
> --- a/gcc/ipa-utils.c
> +++ b/gcc/ipa-utils.c
> @@ -103,8 +103,7 @@ searchc (struct searchc_env* env, struct cgraph_node *v,
>          continue;
>  
>        if (w->aux
> -	  && (avail > AVAIL_INTERPOSABLE
> -	      || avail == AVAIL_INTERPOSABLE))
> +	  && (avail >= AVAIL_INTERPOSABLE))
>  	{
>  	  w_info = (struct ipa_dfs_info *) w->aux;
>  	  if (w_info->new_node)
> diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
> index b70e8c57108..aad08148348 100644
> --- a/gcc/ipa-utils.h
> +++ b/gcc/ipa-utils.h
> @@ -262,6 +262,16 @@ odr_type_p (const_tree t)
>    return false;
>  }
>  
> +/* We only propagate across edges with non-interposable callee.  */
> +
> +inline bool
> +ignore_edge_if_not_available (struct cgraph_edge *e)
> +{
> +  enum availability avail;
> +  e->callee->function_or_virtual_thunk_symbol (&avail, e->caller);
> +  return (avail <= AVAIL_INTERPOSABLE);
> +}
> +
>  #endif  /* GCC_IPA_UTILS_H  */
>  
>  
> -- 
> 2.20.1
>
Jan Hubicka June 10, 2019, 11 a.m. UTC | #7
> Hi.
> 
> IPA pure const should always construct ipa_reduced_postorder with
> possibility to cross AVAIL_INTERPOSABLE boundary. The pass itself
> can then properly stop propagation on these symbols.
> 
> The patch is pre-approved by Honza.
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2019-02-08  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/89009
> 	* ipa-cp.c (build_toporder_info): Remove usage of a param.
> 	* ipa-inline.c (inline_small_functions): Likewise.
> 	* ipa-pure-const.c (propagate_pure_const): Likewise.
> 	(propagate_nothrow): Likewise.
> 	* ipa-reference.c (propagate): Likewise.
> 	* ipa-utils.c (struct searchc_env): Remove unused field.
> 	(searchc): Always search across AVAIL_INTERPOSABLE.
> 	(ipa_reduced_postorder): Always allow AVAIL_INTERPOSABLE as
> 	the only called IPA pure const can properly not propagate
> 	across interposable boundary.
> 	* ipa-utils.h (ipa_reduced_postorder): Remove param.

OK,
thanks!

Honza
diff mbox series

Patch

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 3489ed59ce2..442d5c63eff 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -815,7 +815,7 @@  build_toporder_info (struct ipa_topo_info *topo)
   topo->stack = XCNEWVEC (struct cgraph_node *, symtab->cgraph_count);
 
   gcc_checking_assert (topo->stack_top == 0);
-  topo->nnodes = ipa_reduced_postorder (topo->order, true, true, NULL);
+  topo->nnodes = ipa_reduced_postorder (topo->order, true, NULL);
 }
 
 /* Free information about strongly connected components and the arrays in
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 4ddbfdf772c..360c3de3289 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -1778,7 +1778,7 @@  inline_small_functions (void)
      metrics.  */
 
   max_count = profile_count::uninitialized ();
-  ipa_reduced_postorder (order, true, true, NULL);
+  ipa_reduced_postorder (order, true, NULL);
   free (order);
 
   FOR_EACH_DEFINED_FUNCTION (node)
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 8227eed29bc..a8a3956d2d5 100644
--- a/gcc/ipa-pure-const.c
+++ b/gcc/ipa-pure-const.c
@@ -1422,7 +1422,7 @@  propagate_pure_const (void)
   bool remove_p = false;
   bool has_cdtor;
 
-  order_pos = ipa_reduced_postorder (order, true, false,
+  order_pos = ipa_reduced_postorder (order, true,
 				     ignore_edge_for_pure_const);
   if (dump_file)
     {
@@ -1751,7 +1751,7 @@  propagate_nothrow (void)
   int i;
   struct ipa_dfs_info * w_info;
 
-  order_pos = ipa_reduced_postorder (order, true, false,
+  order_pos = ipa_reduced_postorder (order, true,
 				     ignore_edge_for_nothrow);
   if (dump_file)
     {
diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 8ad12d30bb2..d1759a374bc 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -712,7 +712,7 @@  propagate (void)
      the global information.  All the nodes within a cycle will have
      the same info so we collapse cycles first.  Then we can do the
      propagation in one pass from the leaves to the roots.  */
-  order_pos = ipa_reduced_postorder (order, true, true, ignore_edge_p);
+  order_pos = ipa_reduced_postorder (order, true, ignore_edge_p);
   if (dump_file)
     ipa_print_order (dump_file, "reduced", order, order_pos);
 
diff --git a/gcc/ipa-utils.c b/gcc/ipa-utils.c
index aebcb8ea91d..79b250c3943 100644
--- a/gcc/ipa-utils.c
+++ b/gcc/ipa-utils.c
@@ -63,7 +63,6 @@  struct searchc_env {
   int order_pos;
   splay_tree nodes_marked_new;
   bool reduce;
-  bool allow_overwritable;
   int count;
 };
 
@@ -105,7 +104,7 @@  searchc (struct searchc_env* env, struct cgraph_node *v,
 
       if (w->aux
 	  && (avail > AVAIL_INTERPOSABLE
-	      || (env->allow_overwritable && avail == AVAIL_INTERPOSABLE)))
+	      || avail == AVAIL_INTERPOSABLE))
 	{
 	  w_info = (struct ipa_dfs_info *) w->aux;
 	  if (w_info->new_node)
@@ -162,7 +161,7 @@  searchc (struct searchc_env* env, struct cgraph_node *v,
 
 int
 ipa_reduced_postorder (struct cgraph_node **order,
-		       bool reduce, bool allow_overwritable,
+		       bool reduce,
 		       bool (*ignore_edge) (struct cgraph_edge *))
 {
   struct cgraph_node *node;
@@ -175,15 +174,13 @@  ipa_reduced_postorder (struct cgraph_node **order,
   env.nodes_marked_new = splay_tree_new (splay_tree_compare_ints, 0, 0);
   env.count = 1;
   env.reduce = reduce;
-  env.allow_overwritable = allow_overwritable;
 
   FOR_EACH_DEFINED_FUNCTION (node)
     {
       enum availability avail = node->get_availability ();
 
       if (avail > AVAIL_INTERPOSABLE
-	  || (allow_overwritable
-	      && (avail == AVAIL_INTERPOSABLE)))
+	  || avail == AVAIL_INTERPOSABLE)
 	{
 	  /* Reuse the info if it is already there.  */
 	  struct ipa_dfs_info *info = (struct ipa_dfs_info *) node->aux;
diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
index 618d74f78ee..b70e8c57108 100644
--- a/gcc/ipa-utils.h
+++ b/gcc/ipa-utils.h
@@ -36,7 +36,7 @@  struct ipa_dfs_info {
 
 /* In ipa-utils.c  */
 void ipa_print_order (FILE*, const char *, struct cgraph_node**, int);
-int ipa_reduced_postorder (struct cgraph_node **, bool, bool,
+int ipa_reduced_postorder (struct cgraph_node **, bool,
 			  bool (*ignore_edge) (struct cgraph_edge *));
 void ipa_free_postorder_info (void);
 vec<cgraph_node *> ipa_get_nodes_in_cycle (struct cgraph_node *);