Re: [tree-profiling-branch PATCH IPCP extensions + Function cloning

[Jan Hubicka <jh at suse dot cz>]

> Hello,
> 
> Following my message 
> http://gcc.gnu.org/ml/gcc-patches/2004-10/msg00854.html
> I attach the patch below.
> IPA/CP code has been adopted to the new cfg+gimple representation.
> There's also new code for Fortran (reference) constants and float 
> constants.
> I added a new directory testsuite/gcc.dg/ipa for ipa testcases
> (ipa-1.c, ipa-2.c) and also new scancgraph.exp in testsuite/lib/ 
> to scan the cgraph dump file, which is (temporarily) the ipcp dump file.
> 
> Still not included in this patch:
> - profiling information for versioned functions needs further checks. 
> - ipa passes: the code is still not integrated within this framework.
> 
> Boostrapped with ipa-cp option on POWER4, successfully for C, there's 
> a problem for C++. Ran testsuite only for C and got one failure.
> 
> I decided to submit this patch, although there are some 
> issues to work on, because it already contains many changes and much
> improved functionality. A further patch will correct the remaining 
> issues.
> 
> Thanks,
> Razya
> 
> 
>   


> 2004-10-24  Razya Ladelsky  <razya@il.ibm.com>
> 
>  	* ipa_prop.c: Change/add comments and adjust to GNU standards.
>         (hashtab.h, toplev.h, flags.h, debug.h, target.h, varray.h,
>  	tree-gimple.h): Remove dependency.
>         (ipa_node_create, ipcp_method_is_cloned, ipcp_method_set_orig_node,
>         ipcp_method_orig_node, ipa_cloned_create, ipcp_method_dont_insert_const,
> 	ipa_cloned_create, ipcp_update_callgraph, ipcp_redirect, ipcp_propagate_stage
> 	ipa_edges_create, ipcp_type_is_const, ipcp_asm_walk_tree_1, 
>         ipcp_after_propagate): New functions.
>         (ipcp_cs_get_first, ipcp_cs_get_next,
> 	ipcp_cs_not_last): Remove functions.
>         (ipcp_stage1, ipcp_stage2, ipcp_stage3): Rename to -
>         (ipcp_init_stage, ipcp_iterate_stage, ipcp_insert_stage) Respectively.
>         (ipcp_method_dont_insert_const): Rename to (ipcp_method_dont_propagate).
>         (ipcp_method_compute_modify): check DECL_UNINLINABLE
>         instead of calling tree_inlinable_function_p to find out if a given
> 	function is inlinable.
>         (ipcp_driver): Update.
>  	* ipa_prop.h: Change Comment.
>         (struct ipa_node): Add new fields.
> 	* cgraph.h:  (varray.h): New include.
> 	(update_call_expr, cgraph_copy_node_for_versioning,
>  	cgraph_function_versioning): New declarations.
> 	* Makefile.in (CGRAPH_H): Add dependency to varray.h.
>  	* common.opt: Remove fipa-no-cloning flag.
> 	* cgraphunit.c (cgraph_optimize):
> 	Support for IPA constant propagation.
> 	(update_call_expr, cgraph_copy_node_for_versioning,
> 	cgraph_function_versioning): New functions to support versioning.
> 	* integrate.c: (copy_decl_for_versioning): New function. Copies decl
> 	tree node for the purpose of versioning.
> 	* gimplify.c: (create_function_name): New function.
> 	* tree-gimple.h: (create_function_name): New declaration.
> 	* tree-inline.c: (struct inline_data): New fields to support versioning.
> 	(copy_arguments_for_versioning, version_body,
> 	copy_static_chain, function_versionable_p,
> 	tree_function_versioning, copy_decl_for_versioning):
> 	New functions to support tree
> 	function versioning.
> 	(remap_decl, copy_body_r, copy_cfg_body): Support function versioning.
> 	* tree-inline.h: (tree_versionable_function_p,
> 	tree_function_versioning): New declarations.
>         * common.opt: (fipa-no-cloning): Remove fipa-no-cloning flag.
>         * doc/invoke.texi: (fipa-no-cloning): Same.
>         * testsuite/gcc.dg: (ipa): Add a new directory for ipa.
>         * testsuite/gcc.dg/ipa: (ipa-1.c ipa-2.c): Add new ipcp testcases.
>         * testsuite/lib: (scancgraph.exp): Add new file to scan (the temporary)
> 	ipa/cgraph dump file.
>         * testsuite/lib/gcc-dg.exp: Add loading of scancgraph.exp.
> 
> 
*************** static void cgraph_analyze_function (str
*** 180,186 ****
     record_calls_1.  */
  static struct pointer_set_t *visited_nodes;
  
! static FILE *cgraph_dump_file;
  
  /* Determine if function DECL is needed.  That is, visible to something
     either outside this translation unit, something magic in the system
--- 180,186 ----
     record_calls_1.  */
  static struct pointer_set_t *visited_nodes;
  
! /*static*/ FILE *cgraph_dump_file;

Perhaps rather than exporint the variables, I would preffer the
ipa_driver to be hooked into ipa structures.  You don't need to use the
analyze/modify hooks so you won't run into the pass ordering problems.
We've discussed the analyze/modify issues with Kenneth and Daniel and it
seems to make more sense to avoid the idea for now and revisit this
later when implementing whole program IPA when we have better idea what
data we want to pass down from the early optimization passes.

+ /* Tree versioning hooks.  */
+ #define LANG_HOOKS_TREE_VERSIONING_CANNOT_VERSION_TREE_FN \
+   lhd_tree_versioning_cannot_version_tree_fn

I tend to recall that we concluded that this hook is not needed?  Do you
have testcase?
!       if (!id->versioning_p)
! 	new = make_edge (BASIC_BLOCK (last_basic_block-1), EXIT_BLOCK_PTR,
  		       EDGE_FALLTHRU);
+       else
+ 	{
+ 	  basic_block bb1,bb2;
+ 	  edge e_step;
+ 	
+ 	  bb2 = EXIT_BLOCK_PTR_FOR_FUNCTION(cfun_to_copy);
+ 	  if (bb2->preds)
+ 	    {	     
+ 	      FOR_EACH_EDGE(e_step,ei,bb2->preds)
+ 		{
+ 		  bb1 = e_step->src;
+ 		  new = make_edge (BASIC_BLOCK (bb1->index), EXIT_BLOCK_PTR,
+ 				   e_step->flags);
+ 		}
+ 	    }
+ 	}

How exactly this relate to clonning code?  I think the function clonning
already knows how to duplicate the gimple function with the CFG, so you
should not need any more adjustments here...
*************** copy_tree_r (tree *tp, int *walk_subtree
*** 3076,3081 ****
--- 3128,3162 ----
        tree chain = TREE_CHAIN (*tp);
        tree new;
  
+       if (id && id->versioning_p)
+ 	if (id->ipa_info && VARRAY_ACTIVE_SIZE (id->ipa_info) > 0)
+ 	  {
+ 	    unsigned i;
+ 	   
+ 	    for (i = 0; i < VARRAY_ACTIVE_SIZE (id->ipa_info); i++)
+ 	      {
+ 		struct ipcp_replace_map *replace_info; 
+ 		replace_info = VARRAY_GENERIC_PTR (id->ipa_info, i);
+ 		
+ 		if (replace_info->replace_p && replace_info->ref_p)
+ 		  {
+ 		    tree old_tree = replace_info->old_tree;
+ 		    tree new_tree = replace_info->new_tree;
+ 	     
+ 		    if (TREE_CODE (*tp) == INDIRECT_REF
+ 			&& TREE_OPERAND (*tp, 0) == old_tree)
+ 		      {
+ 			new = copy_node (new_tree);
+ 			*tp = new;
+ 			*walk_subtrees = 0;
+ 			return NULL_TREE; 
+ 		      }
+ 		  }
+ 	      }
+    
+            
+ 	  }
+ 

Would be possible to move such larger chunks to separate functions so we
do something about those gigants we have in tree-inline.c.
It would also enforce you to add some comment to explain what is going
on ;)
        /* Copy the node.  */
        new = copy_node (*tp);
  
*************** declare_inline_vars (tree bind_expr, tre
*** 3303,3307 ****
--- 3384,3669 ----
    add_var_to_bind_expr (bind_expr, vars);
  }
  
+ /* Copy NODE (which must be a DECL).  The DECL originally was in the FROM_FN,
+    but now it will be in the TO_FN. As apposed for to copy_decl_for_inlining ()
+    function; we do not give a special treatment to parm_decl and result_decl.  */ 
+ static tree
+ copy_decl_for_versioning (tree decl, tree from_fn, tree to_fn)

Also it would be really cool to share at least partly this with the
copy_decl_for_inlining and probably move both here from integrate.c so
they are on one place.

+ /* Return true if the function can have a new version.
+    This is a guard for the versioning functionality.  */
+ static bool
+ function_versionable_p (tree fndecl)
+ {
+   if (fndecl == NULL_TREE)
+     return false;
+   /* ??? There are cases where a function is
+      uninlinable but can have a new versions.  */
+   if (lang_hooks.tree_versioning.cannot_version_tree_fn (&fndecl))
+      return false;
+ 
+   if (!tree_inlinable_function_p (fndecl))  
+     return false;

Why versionability is subset of inlinability?  I would guess that
varargs or setjmp are no stoppers for versioning.  It is not major issue
tought.

+    /*cfun = DECL_STRUCT_FUNCTION (old_decl);
+    current_function_decl = new_decl;*/

Please watch the commented out code...

Otherwise the patch looks fine to me, but I would like to see these few
issues resolved before applying the patch.

Thank you and sorry for delayed reply!
Honza