[PING] Function versioning and IPCP extension

Diego Novillo dnovillo@redhat.com
Fri Jul 22 16:11:00 GMT 2005


On Thu, Jul 21, 2005 at 12:24:26PM +0300, Razya Ladelsky wrote:

> Comments are welcome.
> 
OK after addressing the minor corrections below.

> 	* cgraph.h (update_call_expr, cgraph_copy_node_for_versioning,
>         cgraph_function_versioning): New declarations.
> 	* cgraphunit.c: Add include to ipa-prop.h.
>         (update_call_expr, cgraph_copy_node_for_versioning, 
> 	cgraph_function_versioning): New functions.
>         * gimplify.c (create_function_name): New function.
>         * integhrate.c: Remove copy_decl_for_inlining (was replaced by
>
typo, "integhrate".

> 	copy_decl_for_dup in tree-inline.c).
>
ChangeLog entries should not explain the "why", only the "what".
The "why" is explained in the code and/or the message describing
the patch.  So, for this entry, you probably want:

	* integrate.c (copy_decl_for_inlining): Remove.  Update
	all callers.

>         (copy_decl_for_dup): New function (instead of copy_decl_for_inlining, 
>         including versioning).
>
Similarly here, you'd say:

	(copy_decl_for_dup): Rename from copy_decl_for_inlining.
	Add argument VERSIONING.
>         (copy_arguments_for_versioning, copy_static_chain, 
> 	function_versionable_p, tree_versionable_function_p, 
> 	tree_function_versioning, replace_ref_tree): New functions.
>         * tree-inline.h: Include varray.h.
>         (tree_versionable_function_p,  tree_function_versioning, 
> 	tree copy_decl_for_dup): New declarations.
> 
ChangeLog entries are mis-aligned, though that could be the
mailer.

> + void update_call_expr (struct cgraph_node *);
> + struct cgraph_node *cgraph_copy_node_for_versioning (struct cgraph_node *,
> +                                                      tree, varray_type);
>
No need to make these two extern.

> + struct cgraph_node *cgraph_function_versioning (struct cgraph_node *,
> + /* Create a new function name with PREFIX.  Returns an identifier.  */
> + tree
> + create_function_name (const char *prefix)
>
Why not use create_tmp_var_name directly?

> + /* Represent which DECL tree (or reference to such tree)
> +    will be replaced by another tree while versioning.  */
> + struct ipa_replace_map
> + {
> +   /* The tree that will be replaced.  */
> +   tree old_tree;
> +   /* The new (replacing) tree.  */ 
> +   tree new_tree;
> +   /* True when a substitution should be done, false otherwise.  */
> +   bool replace_p;
> +   /* True when we replace a reference to old_tree.  */
> +   bool ref_p;
>
If you make these unsigned bitfields, you'll save a word per
node.  But it's not so important if the structure is transient or
not very large.

> ! /* Build and initialize ipa_replace_map struct
> !    according to TYPE. This struct is read by versioning, which
> !    operates according to the flags sent.  */
> ! static struct ipa_replace_map *
> ! ipcp_replace_map_create (enum cvalue_type type, tree parm_tree,
> ! 			 union parameter_info *cvalue)
>
What's PARM_TREE?  What's CVALUE?

> !       if (ipcp_type_is_const (cval_type))
> ! 	{
> ! 	  jump_func = ipa_callsite_param (cs, i);
> ! 	  type = get_type (jump_func);
> ! 	  if (type != CONST_IPATYPE_INT && type != CONST_IPATYPE_FLOAT
> ! 	      && type != CONST_IPATYPE_INT_REF
> ! 	      && type != CONST_IPATYPE_FLOAT_REF)
>
Line up predicates vertically.

> *************** ipcp_insert_stage (void)
> *** 691,696 ****
> --- 1118,1156 ----
>   	}
>         if (const_param == 0)
>   	continue;
> +       VARRAY_GENERIC_PTR_INIT (replace_trees, const_param, "replace_trees");
>
VECs are slightly better than VARRAYs.  But I don't care enough
to ask you to change them.

> !   /* The new variable/label has no RTL, yet.  */
> !   if (!TREE_STATIC (copy) && !DECL_EXTERNAL (copy))
> !     SET_DECL_RTL (copy, NULL_RTX);
> !   /* These args would always appear unused, if not for this.  */
> !   TREE_USED (copy) = 1;
> ! 
Need a bit of vertical spacing between different sections of code.

> ! /* Return true if the function is allowed to be versioned.
> !    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 be versioned.  */
> !   if (!tree_inlinable_function_p (fndecl))
> !     return false;
> ! 
Example of one or the other?  What would be the additional tests
that an uninlinable function needs to pass so that it can be
versioned?

> !   return true;
> ! }
> ! 
> ! /* Return true if FNDECL is allowed to be versioned.  */
> ! bool
> ! tree_versionable_function_p (tree fndecl)
> ! {
> !   return function_versionable_p (fndecl);
> ! }
> ! 
Hmm?  Just make function_versionable_p extern.

> !   /* Prepare the data structures for the tree copy.  */
> !   memset (&id, 0, sizeof (id));
> !   /* The new version. */
> !   id.node = new_version_node;
> !   /* The old version. */
> !   id.current_node = cgraph_node (old_decl);
> !   id.versioning_p = true;
> !   id.decl_map = splay_tree_new (splay_tree_compare_pointers, NULL, NULL);
> !   id.caller = new_decl;
> !   id.callee = old_decl;
> !   id.callee_cfun = DECL_STRUCT_FUNCTION (old_decl);
> !   current_function_decl = new_decl;
> !   /* Copy the function's static chain.  */
> !   p = DECL_STRUCT_FUNCTION (old_decl)->static_chain_decl;
> !   if (p)
> !     DECL_STRUCT_FUNCTION (new_decl)->static_chain_decl =
> !       copy_static_chain (DECL_STRUCT_FUNCTION (old_decl)->static_chain_decl,
> ! 			 &id);
> !   /* Copy the function's arguments.  */
> !   if (DECL_ARGUMENTS (old_decl) != NULL_TREE)
> !     DECL_ARGUMENTS (new_decl) =
> !       copy_arguments_for_versioning (DECL_ARGUMENTS (old_decl), &id);
> !   /* If there's a tree_map, prepare for substitution.  */
> !   if (tree_map)
> !     for (i = 0; i < VARRAY_ACTIVE_SIZE (tree_map); i++)
> !       {
> ! 	replace_info = VARRAY_GENERIC_PTR (tree_map, i);
> ! 	if (replace_info->replace_p && !replace_info->ref_p)
> ! 	  insert_decl_map (&id, replace_info->old_tree,
> ! 			   replace_info->new_tree);
> ! 	else if (replace_info->replace_p && replace_info->ref_p)
> ! 	  id.ipa_info = tree_map;
> !       }
> !   DECL_INITIAL (new_decl) = remap_blocks (DECL_INITIAL (id.callee), &id);
> !   /* Renumber the lexical scoping (non-code) blocks consecutively.  */
> !   number_blocks (id.caller);
> !   if (DECL_STRUCT_FUNCTION (old_decl)->unexpanded_var_list != NULL_TREE)
> !     /* Add local vars.  */
> !     for (t_step = DECL_STRUCT_FUNCTION (old_decl)->unexpanded_var_list;
> ! 	 t_step; t_step = TREE_CHAIN (t_step))
> !       {
> ! 	tree var = TREE_VALUE (t_step);
> ! 	if (TREE_STATIC (var) && !TREE_ASM_WRITTEN (var))
> ! 	  cfun->unexpanded_var_list = tree_cons (NULL_TREE, var,
> ! 						 cfun->unexpanded_var_list);
> ! 	else
> ! 	  cfun->unexpanded_var_list =
> ! 	    tree_cons (NULL_TREE, remap_decl (var, &id),
> ! 		       cfun->unexpanded_var_list);
> !       }
>
Vertical spacing, please.  This is much too crowded.



More information about the Gcc-patches mailing list