[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