Re: [PATCH] Cleanup do_per_function, require less push/pop_cfun

[Richard Biener <rguenther at suse dot de>]

On Mon, 28 Apr 2014, Jan Hubicka wrote:

> > On Wed, 23 Apr 2014, Richard Biener wrote:
> > 
> > > 
> > > This avoids all the complex work on simple things like
> > > clear_last_verified.  It also makes eventually inlining all
> > > calls (for example the one with the small IPA pass hack)
> > > less code-duplicating.
> > > 
> > > I had to remove the asserts in favor of frees of DOM info in 
> > > release_function_body as the old code released DOM info in
> > > various odd places.
> > > 
> > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> > > 
> > > Honza, does this look ok to you?
> > 
> > I've heard nothing back so I assume it's ok and committed it.
> 
> The purpose of the assert was to make sure that the dominance trees
> are not maintained for whole program at once. release_function_body is
> never called for a function that is "current" and thus the dominators should
> be always freed.
>
> At beggining the datastructure was global and thus it was not possible
> to hold two. I think that is long fixed, but it seems bit wasteful to
> keep all the dominance trees around.  Do you know how dominators leak
> there?

As I didn't really remove sensible calls to free_dominance_info
(but from execute_function_dump, execute_function_todo,
clear_last_verified, verify_curr_properties and 
update_properties_after_pass) I concluded one of those must
be responsible for spuriously freeing dominance info, likely
of functions determined unreachable.

I suspect IPA passes TODO_* to be responsible eventually, or their
transform stage.

I am testing the following.

Richard.

Index: gcc/dominance.c
===================================================================
*** gcc/dominance.c	(revision 209890)
--- gcc/dominance.c	(working copy)
*************** calculate_dominance_info (enum cdi_direc
*** 681,704 ****
  
  /* Free dominance information for direction DIR.  */
  void
! free_dominance_info (enum cdi_direction dir)
  {
    basic_block bb;
    unsigned int dir_index = dom_convert_dir_to_idx (dir);
  
!   if (!dom_info_available_p (dir))
      return;
  
!   FOR_ALL_BB_FN (bb, cfun)
      {
        et_free_tree_force (bb->dom[dir_index]);
        bb->dom[dir_index] = NULL;
      }
    et_free_pools ();
  
!   n_bbs_in_dom_tree[dir_index] = 0;
  
!   dom_computed[dir_index] = DOM_NONE;
  }
  
  /* Return the immediate dominator of basic block BB.  */
--- 681,710 ----
  
  /* Free dominance information for direction DIR.  */
  void
! free_dominance_info (function *fn, enum cdi_direction dir)
  {
    basic_block bb;
    unsigned int dir_index = dom_convert_dir_to_idx (dir);
  
!   if (!dom_info_available_p (fn, dir))
      return;
  
!   FOR_ALL_BB_FN (bb, fn)
      {
        et_free_tree_force (bb->dom[dir_index]);
        bb->dom[dir_index] = NULL;
      }
    et_free_pools ();
  
!   fn->cfg->x_n_bbs_in_dom_tree[dir_index] = 0;
  
!   fn->cfg->x_dom_computed[dir_index] = DOM_NONE;
! }
! 
! void
! free_dominance_info (enum cdi_direction dir)
! {
!   free_dominance_info (cfun, dir);
  }
  
  /* Return the immediate dominator of basic block BB.  */
*************** next_dom_son (enum cdi_direction dir, ba
*** 1461,1471 ****
  /* Return dominance availability for dominance info DIR.  */
  
  enum dom_state
! dom_info_state (enum cdi_direction dir)
  {
    unsigned int dir_index = dom_convert_dir_to_idx (dir);
  
!   return dom_computed[dir_index];
  }
  
  /* Set the dominance availability for dominance info DIR to NEW_STATE.  */
--- 1467,1485 ----
  /* Return dominance availability for dominance info DIR.  */
  
  enum dom_state
! dom_info_state (function *fn, enum cdi_direction dir)
  {
+   if (!fn->cfg)
+     return DOM_NONE;
+ 
    unsigned int dir_index = dom_convert_dir_to_idx (dir);
+   return fn->cfg->x_dom_computed[dir_index];
+ }
  
! enum dom_state
! dom_info_state (enum cdi_direction dir)
! {
!   return dom_info_state (cfun, dir);
  }
  
  /* Set the dominance availability for dominance info DIR to NEW_STATE.  */
*************** set_dom_info_availability (enum cdi_dire
*** 1481,1491 ****
  /* Returns true if dominance information for direction DIR is available.  */
  
  bool
! dom_info_available_p (enum cdi_direction dir)
  {
!   unsigned int dir_index = dom_convert_dir_to_idx (dir);
  
!   return dom_computed[dir_index] != DOM_NONE;
  }
  
  DEBUG_FUNCTION void
--- 1495,1509 ----
  /* Returns true if dominance information for direction DIR is available.  */
  
  bool
! dom_info_available_p (function *fn, enum cdi_direction dir)
  {
!   return dom_info_state (fn, dir) != DOM_NONE;
! }
  
! bool
! dom_info_available_p (enum cdi_direction dir)
! {
!   return dom_info_available_p (cfun, dir);
  }
  
  DEBUG_FUNCTION void
Index: gcc/basic-block.h
===================================================================
*** gcc/basic-block.h	(revision 209890)
--- gcc/basic-block.h	(working copy)
*************** enum cdi_direction
*** 826,835 ****
--- 826,838 ----
    CDI_POST_DOMINATORS = 2
  };
  
+ extern enum dom_state dom_info_state (function *, enum cdi_direction);
  extern enum dom_state dom_info_state (enum cdi_direction);
  extern void set_dom_info_availability (enum cdi_direction, enum dom_state);
+ extern bool dom_info_available_p (function *, enum cdi_direction);
  extern bool dom_info_available_p (enum cdi_direction);
  extern void calculate_dominance_info (enum cdi_direction);
+ extern void free_dominance_info (function *, enum cdi_direction);
  extern void free_dominance_info (enum cdi_direction);
  extern basic_block nearest_common_dominator (enum cdi_direction,
  					     basic_block, basic_block);
Index: gcc/passes.c
===================================================================
*** gcc/passes.c	(revision 209890)
--- gcc/passes.c	(working copy)
*************** execute_function_todo (function *fn, voi
*** 1761,1801 ****
      rebuild_cgraph_edges ();
  
    /* If we've seen errors do not bother running any verifiers.  */
!   if (seen_error ())
      {
-       pop_cfun ();
-       return;
-     }
- 
  #if defined ENABLE_CHECKING
!   if (flags & TODO_verify_ssa)
!     {
!       verify_gimple_in_cfg (cfun);
!       verify_ssa (true);
!     }
!   else if (flags & TODO_verify_stmts)
!     verify_gimple_in_cfg (cfun);
!   if (flags & TODO_verify_flow)
!     verify_flow_info ();
!   if (flags & TODO_verify_il)
!     {
!       if (current_loops
! 	  && loops_state_satisfies_p (LOOP_CLOSED_SSA))
  	{
! 	  if (!(flags & (TODO_verify_stmts|TODO_verify_ssa)))
! 	    verify_gimple_in_cfg (cfun);
! 	  if (!(flags & TODO_verify_ssa))
! 	    verify_ssa (true);
! 	  verify_loop_closed_ssa (false);
  	}
!     }
!   if (flags & TODO_verify_rtl_sharing)
!     verify_rtl_sharing ();
  #endif
  
    fn->last_verified = flags & TODO_verify_all;
  
    pop_cfun ();
  }
  
  /* Perform all TODO actions.  */
--- 1761,1813 ----
      rebuild_cgraph_edges ();
  
    /* If we've seen errors do not bother running any verifiers.  */
!   if (!seen_error ())
      {
  #if defined ENABLE_CHECKING
!       dom_state pre_verify_state = dom_info_state (fn, CDI_DOMINATORS);
!       dom_state pre_verify_pstate = dom_info_state (fn, CDI_POST_DOMINATORS);
! 
!       if (flags & TODO_verify_ssa)
  	{
! 	  verify_gimple_in_cfg (cfun);
! 	  verify_ssa (true);
  	}
!       else if (flags & TODO_verify_stmts)
! 	verify_gimple_in_cfg (cfun);
!       if (flags & TODO_verify_flow)
! 	verify_flow_info ();
!       if (flags & TODO_verify_il)
! 	{
! 	  if (current_loops
! 	      && loops_state_satisfies_p (LOOP_CLOSED_SSA))
! 	    {
! 	      if (!(flags & (TODO_verify_stmts|TODO_verify_ssa)))
! 		verify_gimple_in_cfg (cfun);
! 	      if (!(flags & TODO_verify_ssa))
! 		verify_ssa (true);
! 	      verify_loop_closed_ssa (false);
! 	    }
! 	}
!       if (flags & TODO_verify_rtl_sharing)
! 	verify_rtl_sharing ();
! 
!       /* Make sure verifiers don't change dominator state.  */
!       gcc_assert (dom_info_state (fn, CDI_DOMINATORS) == pre_verify_state);
!       gcc_assert (dom_info_state (fn, CDI_POST_DOMINATORS) == pre_verify_pstate);
  #endif
+     }
  
    fn->last_verified = flags & TODO_verify_all;
  
    pop_cfun ();
+ 
+   /* For IPA passes make sure to release dominator info, it can be
+      computed by non-verifying TODOs.  */
+   if (!cfun)
+     {
+       free_dominance_info (fn, CDI_DOMINATORS);
+       free_dominance_info (fn, CDI_POST_DOMINATORS);
+     }
  }
  
  /* Perform all TODO actions.  */
Index: gcc/cgraph.c
===================================================================
*** gcc/cgraph.c	(revision 209890)
--- gcc/cgraph.c	(working copy)
*************** release_function_body (tree decl)
*** 1696,1703 ****
  	}
        if (cfun->cfg)
  	{
! 	  free_dominance_info (CDI_DOMINATORS);
! 	  free_dominance_info (CDI_POST_DOMINATORS);
  	  clear_edges ();
  	  cfun->cfg = NULL;
  	}
--- 1696,1703 ----
  	}
        if (cfun->cfg)
  	{
! 	  gcc_assert (!dom_info_available_p (CDI_DOMINATORS));
! 	  gcc_assert (!dom_info_available_p (CDI_POST_DOMINATORS));
  	  clear_edges ();
  	  cfun->cfg = NULL;
  	}