diff mbox

Committed: removal of n_edges macro

Message ID 1384879975.11568.117.camel@surprise
State New
Headers show

Commit Message

David Malcolm Nov. 19, 2013, 4:52 p.m. UTC 
On Tue, 2013-11-19 at 09:49 +0100, Richard Biener wrote:
> On Mon, 18 Nov 2013, David Malcolm wrote:
> 
> > On Fri, 2013-11-15 at 20:38 -0500, David Malcolm wrote:
> > > On Wed, 2013-11-13 at 14:44 +0100, Richard Biener wrote:
> > > > On Wed, 13 Nov 2013, David Malcolm wrote:
> > > > 
> > > > > On Wed, 2013-11-13 at 13:53 +0100, Richard Biener wrote:
> > > > > > On Wed, 13 Nov 2013, Martin Jambor wrote:
> > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Wed, Nov 13, 2013 at 10:49:09AM +0100, Jakub Jelinek wrote:
> > > > > > > > Hi!
> > > > > > > > 
> > > > > > > > void f1 (void) {}
> > > > > > > > __attribute__((target ("avx"))) void f2 (void) {}
> > > > > > > > __attribute__((target ("avx2"))) void f3 (void) {}
> > > > > > > > __attribute__((target ("sse3"))) void f4 (void) {}
> > > > > > > > __attribute__((target ("ssse3"))) void f5 (void) {}
> > > > > > > > __attribute__((target ("sse4"))) void f6 (void) {}
> > > > > > > > takes about 3 seconds to compile at -O2, because set_cfun is terribly
> > > > > > > > expensive and there are hundreds of such calls.
> > > > > > > > The following patch is just a quick change to avoid some of them:
> > > > > > > > execute_function_todo starts with:
> > > > > > > >   unsigned int flags = (size_t)data;
> > > > > > > >   flags &= ~cfun->last_verified;
> > > > > > > >   if (!flags)
> > > > > > > >     return;
> > > > > > > > and if flags is initially zero, it does nothing.
> > > > > > > > Similarly, execute_function_dump has the whole body surrounded by
> > > > > > > >   if (dump_file && current_function_decl)
> > > > > > > > and thus if dump_file is NULL, there is nothing to do.
> > > > > > > > So IMHO in neither case (which happens pretty frequently) we need to
> > > > > > > > set_cfun to every function during IPA.
> > > > > > > > 
> > > > > > > > Also, I wonder if we couldn't defer the expensive ira_init, if the info
> > > > > > > > computed by it is used only during RTL optimization passes (haven't verified
> > > > > > > > it yet), then supposedly we could just remember using some target hook
> > > > > > > > what the last state was when we did ira_init last time, and call ira_init
> > > > > > > > again at the start of expansion or so if it is different from the
> > > > > > > > last time.
> > > > > > > 
> > > > > > > I was wondering whether the expensive parts of set_cfun could only be
> > > > > > > run in pass_all_optimizations (and the -Og equivalent) but not when
> > > > > > > changing functions in early and IPA passes.
> > > > > > 
> > > > > > Sounds like a hack ;)
> > > > > > 
> > > > > > Better get things working without the cfun/current_function_decl globals.
> > > > > > Wasn't there someone replacing all implicit uses with explicit ones
> > > > > > for stuff like n_basic_blocks?
> > > > > 
> > > > > I was working on this:
> > > > > http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00780.html
> > > > > though I switched to other tasks I felt were higher priority; sorry.
> > > > > 
> > > > > Do you still want me to go ahead and commit the series of changes you
> > > > > pre-approved there?
> > > > > 
> > > > > i.e. the "n_basic_blocks" macro goes away in favor of:
> > > > >    n_basic_blocks_for_fn (cfun)
> > > > > as a renaming of the existing n_basic_blocks_for_function macro,
> > > > > followed up by analogous changes to the other macros.
> > > > > 
> > > > > Or should I repost before committing?
> > > > 
> > > > I'd say create the n_basic_blocks patch and post it, that gives
> > > > people a chance to object.  If nobody chimes in I approve it
> > > > and pre-approve the rest ;)
> > > > 
> > > > Using n_basic_blocks_for_fn (cfun) might feel backwards if
> > > > eventually we'd want to C++-ify struct function and make
> > > > n_basic_blocks a member function which would make it
> > > > cfun->n_basic_blocks () instead.  Ok, I think that will get
> > > > us into C++ bikeshedding again ;)
> > > 
> > > [I can't face another C vs C++ discussion right now :)]
> > > 
> > > Thanks.  Attached is such a patch, eliminating the:
> > >   n_basic_blocks
> > > macro in favor of
> > >   n_basic_blocks_for_fn (cfun)
> > > 
> > > Successfully bootstrapped on x86_64-unknown-linux-gnu, and successfully
> > > compiled stage1 on spu-unknown-elf and s390-linux-gnu (given that those
> > > config files are affected).
> > > 
> > > Given the conditional pre-approval above, I'm posting here to give
> > > people a change to object - otherwise I'll commit, and followup with the
> > > other macros that implicitly use cfun as per the thread linked to above.
> > 
> > Committed to trunk as r204995; I plan to commit followup patches to
> > remove the other such macros.
> 
> Thanks!

The following removed the "n_edges" macro.  I committed it to trunk as
r205044 having successfully bootstrapped on x86_64-unknown-linux-gnu.

(should I continue to post these patches as I commit them?)

Comments

Richard Biener Nov. 19, 2013, 5:01 p.m. UTC | #1 
David Malcolm <dmalcolm@redhat.com> wrote:
>On Tue, 2013-11-19 at 09:49 +0100, Richard Biener wrote:
>> On Mon, 18 Nov 2013, David Malcolm wrote:
>> 
>> > On Fri, 2013-11-15 at 20:38 -0500, David Malcolm wrote:
>> > > On Wed, 2013-11-13 at 14:44 +0100, Richard Biener wrote:
>> > > > On Wed, 13 Nov 2013, David Malcolm wrote:
>> > > > 
>> > > > > On Wed, 2013-11-13 at 13:53 +0100, Richard Biener wrote:
>> > > > > > On Wed, 13 Nov 2013, Martin Jambor wrote:
>> > > > > > 
>> > > > > > > Hi,
>> > > > > > > 
>> > > > > > > On Wed, Nov 13, 2013 at 10:49:09AM +0100, Jakub Jelinek
>wrote:
>> > > > > > > > Hi!
>> > > > > > > > 
>> > > > > > > > void f1 (void) {}
>> > > > > > > > __attribute__((target ("avx"))) void f2 (void) {}
>> > > > > > > > __attribute__((target ("avx2"))) void f3 (void) {}
>> > > > > > > > __attribute__((target ("sse3"))) void f4 (void) {}
>> > > > > > > > __attribute__((target ("ssse3"))) void f5 (void) {}
>> > > > > > > > __attribute__((target ("sse4"))) void f6 (void) {}
>> > > > > > > > takes about 3 seconds to compile at -O2, because
>set_cfun is terribly
>> > > > > > > > expensive and there are hundreds of such calls.
>> > > > > > > > The following patch is just a quick change to avoid
>some of them:
>> > > > > > > > execute_function_todo starts with:
>> > > > > > > >   unsigned int flags = (size_t)data;
>> > > > > > > >   flags &= ~cfun->last_verified;
>> > > > > > > >   if (!flags)
>> > > > > > > >     return;
>> > > > > > > > and if flags is initially zero, it does nothing.
>> > > > > > > > Similarly, execute_function_dump has the whole body
>surrounded by
>> > > > > > > >   if (dump_file && current_function_decl)
>> > > > > > > > and thus if dump_file is NULL, there is nothing to do.
>> > > > > > > > So IMHO in neither case (which happens pretty
>frequently) we need to
>> > > > > > > > set_cfun to every function during IPA.
>> > > > > > > > 
>> > > > > > > > Also, I wonder if we couldn't defer the expensive
>ira_init, if the info
>> > > > > > > > computed by it is used only during RTL optimization
>passes (haven't verified
>> > > > > > > > it yet), then supposedly we could just remember using
>some target hook
>> > > > > > > > what the last state was when we did ira_init last time,
>and call ira_init
>> > > > > > > > again at the start of expansion or so if it is
>different from the
>> > > > > > > > last time.
>> > > > > > > 
>> > > > > > > I was wondering whether the expensive parts of set_cfun
>could only be
>> > > > > > > run in pass_all_optimizations (and the -Og equivalent)
>but not when
>> > > > > > > changing functions in early and IPA passes.
>> > > > > > 
>> > > > > > Sounds like a hack ;)
>> > > > > > 
>> > > > > > Better get things working without the
>cfun/current_function_decl globals.
>> > > > > > Wasn't there someone replacing all implicit uses with
>explicit ones
>> > > > > > for stuff like n_basic_blocks?
>> > > > > 
>> > > > > I was working on this:
>> > > > > http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00780.html
>> > > > > though I switched to other tasks I felt were higher priority;
>sorry.
>> > > > > 
>> > > > > Do you still want me to go ahead and commit the series of
>changes you
>> > > > > pre-approved there?
>> > > > > 
>> > > > > i.e. the "n_basic_blocks" macro goes away in favor of:
>> > > > >    n_basic_blocks_for_fn (cfun)
>> > > > > as a renaming of the existing n_basic_blocks_for_function
>macro,
>> > > > > followed up by analogous changes to the other macros.
>> > > > > 
>> > > > > Or should I repost before committing?
>> > > > 
>> > > > I'd say create the n_basic_blocks patch and post it, that gives
>> > > > people a chance to object.  If nobody chimes in I approve it
>> > > > and pre-approve the rest ;)
>> > > > 
>> > > > Using n_basic_blocks_for_fn (cfun) might feel backwards if
>> > > > eventually we'd want to C++-ify struct function and make
>> > > > n_basic_blocks a member function which would make it
>> > > > cfun->n_basic_blocks () instead.  Ok, I think that will get
>> > > > us into C++ bikeshedding again ;)
>> > > 
>> > > [I can't face another C vs C++ discussion right now :)]
>> > > 
>> > > Thanks.  Attached is such a patch, eliminating the:
>> > >   n_basic_blocks
>> > > macro in favor of
>> > >   n_basic_blocks_for_fn (cfun)
>> > > 
>> > > Successfully bootstrapped on x86_64-unknown-linux-gnu, and
>successfully
>> > > compiled stage1 on spu-unknown-elf and s390-linux-gnu (given that
>those
>> > > config files are affected).
>> > > 
>> > > Given the conditional pre-approval above, I'm posting here to
>give
>> > > people a change to object - otherwise I'll commit, and followup
>with the
>> > > other macros that implicitly use cfun as per the thread linked to
>above.
>> > 
>> > Committed to trunk as r204995; I plan to commit followup patches to
>> > remove the other such macros.
>> 
>> Thanks!
>
>The following removed the "n_edges" macro.  I committed it to trunk as
>r205044 having successfully bootstrapped on x86_64-unknown-linux-gnu.
>
>(should I continue to post these patches as I commit them?)

Yes.

Thanks,
Richard.
diff mbox

Patch

From dc32f3cec5bf56cf071a5a7f7b926c2c26d3fe82 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 18 Nov 2013 20:23:36 -0500
Subject: [PATCH 1/2] Eliminate n_edges macro

gcc/

	* basic-block.h (n_edges_for_function): Rename macro to...
	(n_edges_for_fn): ...this.
	(n_edges): Eliminate macro as work towards making uses of
	cfun be explicit.

	* cfg.c (init_flow): Update for renaming of "n_edges_for_function"
	to "n_edges_for_fn".

	* cfg.c (unchecked_make_edge): Remove usage of n_edges macro.
	(clear_edges): Likewise.
	(free_edge): Likewise.
	* cfghooks.c (dump_flow_info): Likewise.
	* cprop.c (is_too_expensive): Likewise.
	* df-core.c (df_worklist_dataflow_doublequeue): Likewise.
	* gcse.c (is_too_expensive): Likewise.
	(prune_insertions_deletions): Likewise.
	* mcf.c (create_fixup_graph): Likewise.
	* sched-rgn.c (haifa_find_rgns): Likewise.
	* tree-cfg.c (gimple_dump_cfg): Likewise.
	* var-tracking.c (variable_tracking_main_1): Likewise.
---
 gcc/basic-block.h  | 3 +--
 gcc/cfg.c          | 8 ++++----
 gcc/cfghooks.c     | 2 +-
 gcc/cprop.c        | 4 ++--
 gcc/df-core.c      | 2 +-
 gcc/gcse.c         | 8 ++++----
 gcc/mcf.c          | 9 ++++++---
 gcc/sched-rgn.c    | 2 +-
 gcc/tree-cfg.c     | 3 ++-
 gcc/var-tracking.c | 2 +-
 10 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/gcc/basic-block.h b/gcc/basic-block.h
index d247d4f..38391be 100644
--- a/gcc/basic-block.h
+++ b/gcc/basic-block.h
@@ -316,7 +316,7 @@  struct GTY(()) control_flow_graph {
 #define EXIT_BLOCK_PTR_FOR_FUNCTION(FN)	     ((FN)->cfg->x_exit_block_ptr)
 #define basic_block_info_for_function(FN)    ((FN)->cfg->x_basic_block_info)
 #define n_basic_blocks_for_fn(FN)	     ((FN)->cfg->x_n_basic_blocks)
-#define n_edges_for_function(FN)	     ((FN)->cfg->x_n_edges)
+#define n_edges_for_fn(FN)		     ((FN)->cfg->x_n_edges)
 #define last_basic_block_for_function(FN)    ((FN)->cfg->x_last_basic_block)
 #define label_to_block_map_for_function(FN)  ((FN)->cfg->x_label_to_block_map)
 #define profile_status_for_function(FN)	     ((FN)->cfg->x_profile_status)
@@ -330,7 +330,6 @@  struct GTY(()) control_flow_graph {
 #define ENTRY_BLOCK_PTR		(cfun->cfg->x_entry_block_ptr)
 #define EXIT_BLOCK_PTR		(cfun->cfg->x_exit_block_ptr)
 #define basic_block_info	(cfun->cfg->x_basic_block_info)
-#define n_edges			(cfun->cfg->x_n_edges)
 #define last_basic_block	(cfun->cfg->x_last_basic_block)
 #define label_to_block_map	(cfun->cfg->x_label_to_block_map)
 #define profile_status		(cfun->cfg->x_profile_status)
diff --git a/gcc/cfg.c b/gcc/cfg.c
index 10791a7..166ad38 100644
--- a/gcc/cfg.c
+++ b/gcc/cfg.c
@@ -69,7 +69,7 @@  init_flow (struct function *the_fun)
 {
   if (!the_fun->cfg)
     the_fun->cfg = ggc_alloc_cleared_control_flow_graph ();
-  n_edges_for_function (the_fun) = 0;
+  n_edges_for_fn (the_fun) = 0;
   ENTRY_BLOCK_PTR_FOR_FUNCTION (the_fun)
     = ggc_alloc_cleared_basic_block_def ();
   ENTRY_BLOCK_PTR_FOR_FUNCTION (the_fun)->index = ENTRY_BLOCK;
@@ -88,7 +88,7 @@  init_flow (struct function *the_fun)
 static void
 free_edge (edge e)
 {
-  n_edges--;
+  n_edges_for_fn (cfun)--;
   ggc_free (e);
 }
 
@@ -114,7 +114,7 @@  clear_edges (void)
   vec_safe_truncate (EXIT_BLOCK_PTR->preds, 0);
   vec_safe_truncate (ENTRY_BLOCK_PTR->succs, 0);
 
-  gcc_assert (!n_edges);
+  gcc_assert (!n_edges_for_fn (cfun));
 }
 
 /* Allocate memory for basic_block.  */
@@ -262,7 +262,7 @@  unchecked_make_edge (basic_block src, basic_block dst, int flags)
 {
   edge e;
   e = ggc_alloc_cleared_edge_def ();
-  n_edges++;
+  n_edges_for_fn (cfun)++;
 
   e->src = src;
   e->dest = dst;
diff --git a/gcc/cfghooks.c b/gcc/cfghooks.c
index 3016c54..20b90bf 100644
--- a/gcc/cfghooks.c
+++ b/gcc/cfghooks.c
@@ -324,7 +324,7 @@  dump_flow_info (FILE *file, int flags)
   basic_block bb;
 
   fprintf (file, "\n%d basic blocks, %d edges.\n", n_basic_blocks_for_fn (cfun),
-	   n_edges);
+	   n_edges_for_fn (cfun));
   FOR_ALL_BB (bb)
     dump_bb (file, bb, 0, flags);
 
diff --git a/gcc/cprop.c b/gcc/cprop.c
index 78cfeba..35a44f2 100644
--- a/gcc/cprop.c
+++ b/gcc/cprop.c
@@ -1729,12 +1729,12 @@  is_too_expensive (const char *pass)
      which have a couple switch statements.  Rather than simply
      threshold the number of blocks, uses something with a more
      graceful degradation.  */
-  if (n_edges > 20000 + n_basic_blocks_for_fn (cfun) * 4)
+  if (n_edges_for_fn (cfun) > 20000 + n_basic_blocks_for_fn (cfun) * 4)
     {
       warning (OPT_Wdisabled_optimization,
 	       "%s: %d basic blocks and %d edges/basic block",
 	       pass, n_basic_blocks_for_fn (cfun),
-	       n_edges / n_basic_blocks_for_fn (cfun));
+	       n_edges_for_fn (cfun) / n_basic_blocks_for_fn (cfun));
 
       return true;
     }
diff --git a/gcc/df-core.c b/gcc/df-core.c
index 20d6c4e..37876af 100644
--- a/gcc/df-core.c
+++ b/gcc/df-core.c
@@ -1097,7 +1097,7 @@  df_worklist_dataflow_doublequeue (struct dataflow *dataflow,
     fprintf (dump_file, "df_worklist_dataflow_doublequeue:"
 	     "n_basic_blocks %d n_edges %d"
 	     " count %d (%5.2g)\n",
-	     n_basic_blocks_for_fn (cfun), n_edges,
+	     n_basic_blocks_for_fn (cfun), n_edges_for_fn (cfun),
 	     dcount, dcount / (float)n_basic_blocks_for_fn (cfun));
 }
 
diff --git a/gcc/gcse.c b/gcc/gcse.c
index 5ed99bd..a37ac6b 100644
--- a/gcc/gcse.c
+++ b/gcc/gcse.c
@@ -1964,7 +1964,7 @@  prune_insertions_deletions (int n_elems)
 
   /* Iterate over the edges counting the number of times each expression
      needs to be inserted.  */
-  for (i = 0; i < (unsigned) n_edges; i++)
+  for (i = 0; i < (unsigned) n_edges_for_fn (cfun); i++)
     {
       EXECUTE_IF_SET_IN_BITMAP (pre_insert_map[i], 0, j, sbi)
 	insertions[j]++;
@@ -1990,7 +1990,7 @@  prune_insertions_deletions (int n_elems)
   /* Now prune PRE_INSERT_MAP and PRE_DELETE_MAP based on PRUNE_EXPRS.  */
   EXECUTE_IF_SET_IN_BITMAP (prune_exprs, 0, j, sbi)
     {
-      for (i = 0; i < (unsigned) n_edges; i++)
+      for (i = 0; i < (unsigned) n_edges_for_fn (cfun); i++)
 	bitmap_clear_bit (pre_insert_map[i], j);
 
       for (i = 0; i < (unsigned) last_basic_block; i++)
@@ -4069,12 +4069,12 @@  is_too_expensive (const char *pass)
      which have a couple switch statements.  Rather than simply
      threshold the number of blocks, uses something with a more
      graceful degradation.  */
-  if (n_edges > 20000 + n_basic_blocks_for_fn (cfun) * 4)
+  if (n_edges_for_fn (cfun) > 20000 + n_basic_blocks_for_fn (cfun) * 4)
     {
       warning (OPT_Wdisabled_optimization,
 	       "%s: %d basic blocks and %d edges/basic block",
 	       pass, n_basic_blocks_for_fn (cfun),
-	       n_edges / n_basic_blocks_for_fn (cfun));
+	       n_edges_for_fn (cfun) / n_basic_blocks_for_fn (cfun));
 
       return true;
     }
diff --git a/gcc/mcf.c b/gcc/mcf.c
index e0e40d8..45adda3 100644
--- a/gcc/mcf.c
+++ b/gcc/mcf.c
@@ -472,11 +472,13 @@  create_fixup_graph (fixup_graph_type *fixup_graph)
 
   /* Each basic_block will be split into 2 during vertex transformation.  */
   int fnum_vertices_after_transform =  2 * n_basic_blocks_for_fn (cfun);
-  int fnum_edges_after_transform = n_edges + n_basic_blocks_for_fn (cfun);
+  int fnum_edges_after_transform =
+    n_edges_for_fn (cfun) + n_basic_blocks_for_fn (cfun);
 
   /* Count the new SOURCE and EXIT vertices to be added.  */
   int fmax_num_vertices =
-    fnum_vertices_after_transform + n_edges + n_basic_blocks_for_fn (cfun) + 2;
+    (fnum_vertices_after_transform + n_edges_for_fn (cfun)
+     + n_basic_blocks_for_fn (cfun) + 2);
 
   /* In create_fixup_graph: Each basic block and edge can be split into 3
      edges. Number of balance edges = n_basic_blocks. So after
@@ -486,7 +488,8 @@  create_fixup_graph (fixup_graph_type *fixup_graph)
      max_edges = 2 * (4 * n_basic_blocks + 3 * n_edges)
      = 8 * n_basic_blocks + 6 * n_edges
      < 8 * n_basic_blocks + 8 * n_edges.  */
-  int fmax_num_edges = 8 * (n_basic_blocks_for_fn (cfun) + n_edges);
+  int fmax_num_edges = 8 * (n_basic_blocks_for_fn (cfun) +
+			    n_edges_for_fn (cfun));
 
   /* Initial num of vertices in the fixup graph.  */
   fixup_graph->num_vertices = n_basic_blocks_for_fn (cfun);
diff --git a/gcc/sched-rgn.c b/gcc/sched-rgn.c
index 20c29c5..87042dd 100644
--- a/gcc/sched-rgn.c
+++ b/gcc/sched-rgn.c
@@ -643,7 +643,7 @@  haifa_find_rgns (void)
   /* Allocate and initialize variables for the first traversal.  */
   max_hdr = XNEWVEC (int, last_basic_block);
   dfs_nr = XCNEWVEC (int, last_basic_block);
-  stack = XNEWVEC (edge_iterator, n_edges);
+  stack = XNEWVEC (edge_iterator, n_edges_for_fn (cfun));
 
   inner = sbitmap_alloc (last_basic_block);
   bitmap_ones (inner);
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index c30b113..d2af39e 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -2106,7 +2106,8 @@  gimple_dump_cfg (FILE *file, int flags)
     {
       dump_function_header (file, current_function_decl, flags);
       fprintf (file, ";; \n%d basic blocks, %d edges, last basic block %d.\n\n",
-	       n_basic_blocks_for_fn (cfun), n_edges, last_basic_block);
+	       n_basic_blocks_for_fn (cfun), n_edges_for_fn (cfun),
+	       last_basic_block);
 
       brief_dump_cfg (file, flags | TDF_COMMENT);
       fprintf (file, "\n");
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index a569d46..cfda63a 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -10161,7 +10161,7 @@  variable_tracking_main_1 (void)
     }
 
   if (n_basic_blocks_for_fn (cfun) > 500 &&
-      n_edges / n_basic_blocks_for_fn (cfun) >= 20)
+      n_edges_for_fn (cfun) / n_basic_blocks_for_fn (cfun) >= 20)
     {
       vt_debug_insns_local (true);
       return 0;
-- 
1.7.11.7