| Message ID | 20110610145543.GF28776@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
Hi, On Fri, Jun 10, 2011 at 04:55:43PM +0200, Jan Hubicka wrote: > Hi, > this patch updated ipa-cp and ipa-prop for aliases. It is basically > easy - we don't analyze nodes represneting aliases and when > propagating we skip them, like everywhere else. > > There are two problems I noticed. First we should not propagate > through calls that are overwritable. When non-overwritable function > A has overwritable alias B, we should propagate through calls to A, > but not throug calls to B. As discussed on IRC it is probably best > to zap the jump functions in question at IPA stage (because at > analysis stage it is not clear at all if the alias will end up > overwritable with LTO). > OK. Nevertheless, I'd prefer to do this in context of the new IPA-CP. > Similar problem already exists with code in > ipa_compute_jump_functions that looks into a callee that might > change with LTO. I either don't understand or fail to see how this is different from the first problem. We even compute jump functions of indirect edges precisely because we hope they will be changed later on... > Index: ipa-cp.c > =================================================================== > --- ipa-cp.c (revision 174905) > +++ ipa-cp.c (working copy) > @@ -818,7 +828,7 @@ ipcp_iterate_stage (void) > /* Some lattices have changed from IPA_TOP to IPA_BOTTOM. > This change should be propagated. */ > { > - gcc_assert (n_cloning_candidates); > + /*gcc_assert (n_cloning_candidates);*/ > ipcp_propagate_stage (); > } > if (dump_file) I know this assert can be horribly irritating but so far it has been very useful at spotting all kinds of errors at various places. (In fact, you added it :-) But as I want to get the whole IPA-CP replaced, I don't care all that much. Martin
> Hi, > > On Fri, Jun 10, 2011 at 04:55:43PM +0200, Jan Hubicka wrote: > > Hi, > > > this patch updated ipa-cp and ipa-prop for aliases. It is basically > > easy - we don't analyze nodes represneting aliases and when > > propagating we skip them, like everywhere else. > > > > There are two problems I noticed. First we should not propagate > > through calls that are overwritable. When non-overwritable function > > A has overwritable alias B, we should propagate through calls to A, > > but not throug calls to B. As discussed on IRC it is probably best > > to zap the jump functions in question at IPA stage (because at > > analysis stage it is not clear at all if the alias will end up > > overwritable with LTO). > > > > OK. Nevertheless, I'd prefer to do this in context of the new > IPA-CP. Yep, lets see how much work will be to get new ipa-cp in. I will try to look into it more this week. > > > Similar problem already exists with code in > > ipa_compute_jump_functions that looks into a callee that might > > change with LTO. > > I either don't understand or fail to see how this is different from > the first problem. We even compute jump functions of indirect edges > precisely because we hope they will be changed later on... Well, it is sort of similar. What is new that same function can be now called in overwritable and non-overwritable way. The old problem is that what used to be overwritable at analysis time is not neccesarily so at WPA time. > > > > Index: ipa-cp.c > > =================================================================== > > --- ipa-cp.c (revision 174905) > > +++ ipa-cp.c (working copy) > > @@ -818,7 +828,7 @@ ipcp_iterate_stage (void) > > /* Some lattices have changed from IPA_TOP to IPA_BOTTOM. > > This change should be propagated. */ > > { > > - gcc_assert (n_cloning_candidates); > > + /*gcc_assert (n_cloning_candidates);*/ > > ipcp_propagate_stage (); > > } > > if (dump_file) > > > I know this assert can be horribly irritating but so far it has been > very useful at spotting all kinds of errors at various places. (In > fact, you added it :-) > > But as I want to get the whole IPA-CP replaced, I don't care all that > much. I am pretty sure I didn't meant to remove it and had the whole thing tested with the assert. The issue was that once aliases are skipped by propagate stage, the lattices stay top. But I already fixed it elsewhere, so I will regtest reverting that hunk. Thanks for pointing this out! Honza > > Martin
> > Index: ipa-cp.c > > =================================================================== > > --- ipa-cp.c (revision 174905) > > +++ ipa-cp.c (working copy) > > @@ -818,7 +828,7 @@ ipcp_iterate_stage (void) > > /* Some lattices have changed from IPA_TOP to IPA_BOTTOM. > > This change should be propagated. */ > > { > > - gcc_assert (n_cloning_candidates); > > + /*gcc_assert (n_cloning_candidates);*/ > > ipcp_propagate_stage (); > > } > > if (dump_file) > > > I know this assert can be horribly irritating but so far it has been > very useful at spotting all kinds of errors at various places. (In > fact, you added it :-) > > But as I want to get the whole IPA-CP replaced, I don't care all that > much. I reverted this change now. Thanks, Honza /bin/bash: :q: command not found
On Jun 10, 2011, at 6:55 PM, Jan Hubicka wrote: > Hi, > this patch updated ipa-cp and ipa-prop for aliases. It is basically easy - we don't > analyze nodes represneting aliases and when propagating we skip them, like everywhere > else. ... > @@ -759,7 +768,8 @@ ipcp_propagate_stage (void) > > for (cs = node->callees; cs; cs = cs->next_callee) > { > - struct ipa_node_params *callee_info = IPA_NODE_REF (cs->callee); > + struct cgraph_node *callee = cgraph_function_or_thunk_node (cs->callee, NULL); > + struct ipa_node_params *callee_info = IPA_NODE_REF (callee); > struct ipa_edge_args *args = IPA_EDGE_REF (cs); > > if (ipa_is_called_with_var_arguments (callee_info) > @@ -778,11 +788,11 @@ ipcp_propagate_stage (void) > { > dest_lat->type = new_lat.type; > dest_lat->constant = new_lat.constant; > - ipa_push_func_to_list (&wl, cs->callee); > + ipa_push_func_to_list (&wl, callee); > } > > if (ipcp_propagate_types (info, callee_info, jump_func, i)) > - ipa_push_func_to_list (&wl, cs->callee); > + ipa_push_func_to_list (&wl, callee); > } > } > } Jan, I have a question about the above hunk. With this hunk you replace all uses of 'cs->callee' with 'callee' in ipcp_propagate_stage() except for in the check: if (ipa_is_called_with_var_arguments (callee_info) || !cs->callee->analyzed || ipa_is_called_with_var_arguments (callee_info)) continue; Is there a reason why you left 'cs->callee' intact in this case? Thank you, -- Maxim Kuvyrkov CodeSourcery / Mentor Graphics
Index: ipa-cp.c =================================================================== --- ipa-cp.c (revision 174905) +++ ipa-cp.c (working copy) @@ -350,6 +350,10 @@ ipcp_versionable_function_p (struct cgra { struct cgraph_edge *edge; + /* We always version the actual function and redirect through the aliases. */ + if (node->alias) + return false; + /* There are a number of generic reasons functions cannot be versioned. We also cannot remove parameters if there are type attributes such as fnspec present. */ @@ -358,7 +362,8 @@ ipcp_versionable_function_p (struct cgra return false; /* Removing arguments doesn't work if the function takes varargs - or use __builtin_apply_args. */ + or use __builtin_apply_args. + FIXME: handle this together with can_change_signature flag. */ for (edge = node->callees; edge; edge = edge->next_callee) { tree t = edge->callee->decl; @@ -380,6 +385,10 @@ ipcp_cloning_candidate_p (struct cgraph_ gcov_type direct_call_sum = 0; struct cgraph_edge *e; + /* We look through aliases, so we clone the aliased function instead. */ + if (node->alias) + return false; + /* We never clone functions that are not visible from outside. FIXME: in future we should clone such functions when they are called with different constants, but current ipcp implementation is not good on this. @@ -498,7 +507,7 @@ ipcp_initialize_node_lattices (struct cg struct ipa_node_params *info = IPA_NODE_REF (node); enum ipa_lattice_type type; - if (ipa_is_called_with_var_arguments (info)) + if (ipa_is_called_with_var_arguments (info) || node->alias) type = IPA_BOTTOM; else if (node->local.local) type = IPA_TOP; @@ -759,7 +768,8 @@ ipcp_propagate_stage (void) for (cs = node->callees; cs; cs = cs->next_callee) { - struct ipa_node_params *callee_info = IPA_NODE_REF (cs->callee); + struct cgraph_node *callee = cgraph_function_or_thunk_node (cs->callee, NULL); + struct ipa_node_params *callee_info = IPA_NODE_REF (callee); struct ipa_edge_args *args = IPA_EDGE_REF (cs); if (ipa_is_called_with_var_arguments (callee_info) @@ -778,11 +788,11 @@ ipcp_propagate_stage (void) { dest_lat->type = new_lat.type; dest_lat->constant = new_lat.constant; - ipa_push_func_to_list (&wl, cs->callee); + ipa_push_func_to_list (&wl, callee); } if (ipcp_propagate_types (info, callee_info, jump_func, i)) - ipa_push_func_to_list (&wl, cs->callee); + ipa_push_func_to_list (&wl, callee); } } } @@ -818,7 +828,7 @@ ipcp_iterate_stage (void) /* Some lattices have changed from IPA_TOP to IPA_BOTTOM. This change should be propagated. */ { - gcc_assert (n_cloning_candidates); + /*gcc_assert (n_cloning_candidates);*/ ipcp_propagate_stage (); } if (dump_file) @@ -946,7 +956,8 @@ ipcp_need_redirect_p (struct cgraph_edge { struct ipa_node_params *orig_callee_info; int i, count; - struct cgraph_node *node = cs->callee, *orig; + struct cgraph_node *node = cgraph_function_or_thunk_node (cs->callee, NULL); + struct cgraph_node *orig; if (!n_cloning_candidates) return false; @@ -1293,7 +1304,7 @@ ipcp_insert_stage (void) int i; VEC (cgraph_edge_p, heap) * redirect_callers; VEC (ipa_replace_map_p,gc)* replace_trees; - int node_callers, count; + int count; tree parm_tree; struct ipa_replace_map *replace_param; fibheap_t heap; @@ -1307,13 +1318,12 @@ ipcp_insert_stage (void) dead_nodes = BITMAP_ALLOC (NULL); - for (node = cgraph_nodes; node; node = node->next) - if (node->analyzed) - { - if (node->count > max_count) - max_count = node->count; - overall_size += inline_summary (node)->self_size; - } + FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (node) + { + if (node->count > max_count) + max_count = node->count; + overall_size += inline_summary (node)->self_size; + } max_new_size = overall_size; if (max_new_size < PARAM_VALUE (PARAM_LARGE_UNIT_INSNS)) @@ -1413,14 +1423,7 @@ ipcp_insert_stage (void) if (!cs && cgraph_will_be_removed_from_program_if_no_direct_calls (node)) bitmap_set_bit (dead_nodes, node->uid); - /* Compute how many callers node has. */ - node_callers = 0; - for (cs = node->callers; cs != NULL; cs = cs->next_caller) - node_callers++; - redirect_callers = VEC_alloc (cgraph_edge_p, heap, node_callers); - for (cs = node->callers; cs != NULL; cs = cs->next_caller) - if (!cs->indirect_inlining_edge) - VEC_quick_push (cgraph_edge_p, redirect_callers, cs); + redirect_callers = collect_callers_of_node (node); /* Redirecting all the callers of the node to the new versioned node. */ @@ -1452,13 +1455,16 @@ ipcp_insert_stage (void) dump_function_to_file (node1->decl, dump_file, dump_flags); for (cs = node->callees; cs; cs = cs->next_callee) - if (cs->callee->aux) - { - fibheap_delete_node (heap, (fibnode_t) cs->callee->aux); - cs->callee->aux = fibheap_insert (heap, - ipcp_estimate_cloning_cost (cs->callee), - cs->callee); - } + { + struct cgraph_node *callee = cgraph_function_or_thunk_node (cs->callee, NULL); + if (callee->aux) + { + fibheap_delete_node (heap, (fibnode_t) callee->aux); + callee->aux = fibheap_insert (heap, + ipcp_estimate_cloning_cost (callee), + callee); + } + } } while (!fibheap_empty (heap)) Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 174905) +++ ipa-prop.c (working copy) @@ -93,7 +93,7 @@ ipa_init_func_list (void) wl = NULL; for (node = cgraph_nodes; node; node = node->next) - if (node->analyzed) + if (node->analyzed && !node->alias) { struct ipa_node_params *info = IPA_NODE_REF (node); /* Unreachable nodes should have been eliminated before ipcp and @@ -1096,6 +1096,7 @@ ipa_compute_jump_functions (struct cgrap for (cs = node->callees; cs; cs = cs->next_callee) { + struct cgraph_node *callee = cgraph_function_or_thunk_node (cs->callee, NULL); /* We do not need to bother analyzing calls to unknown functions unless they may become known during lto/whopr. */ if (!cs->callee->analyzed && !flag_lto) @@ -1103,11 +1104,11 @@ ipa_compute_jump_functions (struct cgrap ipa_count_arguments (cs); /* If the descriptor of the callee is not initialized yet, we have to do it now. */ - if (cs->callee->analyzed) - ipa_initialize_node_params (cs->callee); + if (callee->analyzed) + ipa_initialize_node_params (callee); if (ipa_get_cs_argument_count (IPA_EDGE_REF (cs)) - != ipa_get_param_count (IPA_NODE_REF (cs->callee))) - ipa_set_called_with_variable_arg (IPA_NODE_REF (cs->callee)); + != ipa_get_param_count (IPA_NODE_REF (callee))) + ipa_set_called_with_variable_arg (IPA_NODE_REF (callee)); ipa_compute_jump_functions_for_edge (parms_info, cs); }