| Message ID | ri65yegimym.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] ipa-cp: Write transformation summaries of all functions | expand |
> 2022-11-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/103227 > * ipa-param-manipulation.h (class ipa_param_adjustments): Removed > member function get_updated_index_or_split. > (class ipa_param_body_adjustments): New overload of > register_replacement, new member function append_init_stmts, new > member m_split_agg_csts_inits. > * ipa-param-manipulation.cc: Include ipa-prop.h. > (ipa_param_adjustments::get_updated_index_or_split): Removed. > (ipa_param_body_adjustments::register_replacement): New overload, use > it from the older one. > (ipa_param_body_adjustments::common_initialization): Added the > capability to create replacements for conflicting IPA-CP discovered > constants. > (ipa_param_body_adjustments::ipa_param_body_adjustments): Construct > the new member. > (ipa_param_body_adjustments::append_init_stmts): New function. > * ipa-sra.cc: Include ipa-prop.h. > (push_param_adjustments_for_index): Require IPA-CP transformation > summary as a parameter, do not create replacements which are known to > have constant values. > (process_isra_node_results): Find and pass to the above function the > IPA-CP transformation summary. > * ipa-prop.cc (adjust_agg_replacement_values): Remove the > functionality replacing IPA-SRA created scalar parameters with > constants. Simplify, do not require parameter descriptors, do not > return anything. > (ipcp_transform_function): Simplify now that > adjust_agg_replacement_values does not change cfg. Move definition > and initialization of descriptors lower. > * tree-inline.cc (tree_function_versioning): Call append_init_stmts of > param_body_adjs, if there are any. OK, Honza > > gcc/testsuite/ChangeLog: > > 2022-11-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/103227 > PR ipa/107640 > * gcc.dg/ipa/pr107640-2.c: New test. > --- > gcc/ipa-param-manipulation.cc | 155 ++++++++++++++++---------- > gcc/ipa-param-manipulation.h | 19 ++-- > gcc/ipa-prop.cc | 66 ++++------- > gcc/ipa-sra.cc | 30 ++++- > gcc/testsuite/gcc.dg/ipa/pr107640-2.c | 50 +++++++++ > gcc/tree-inline.cc | 2 + > 6 files changed, 208 insertions(+), 114 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr107640-2.c > > diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc > index cee0e23f946..e92cfc0b6d5 100644 > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -46,7 +46,7 @@ along with GCC; see the file COPYING3. If not see > #include "tree-phinodes.h" > #include "cfgexpand.h" > #include "attribs.h" > - > +#include "ipa-prop.h" > > /* Actual prefixes of different newly synthetized parameters. Keep in sync > with IPA_PARAM_PREFIX_* defines. */ > @@ -449,39 +449,6 @@ ipa_param_adjustments::get_updated_indices (vec<int> *new_indices) > } > } > > -/* If a parameter with original INDEX has survived intact, return its new > - index. Otherwise return -1. In that case, if it has been split and there > - is a new parameter representing a portion at unit OFFSET for which a value > - of a TYPE can be substituted, store its new index into SPLIT_INDEX, > - otherwise store -1 there. */ > -int > -ipa_param_adjustments::get_updated_index_or_split (int index, > - unsigned unit_offset, > - tree type, int *split_index) > -{ > - unsigned adj_len = vec_safe_length (m_adj_params); > - for (unsigned i = 0; i < adj_len ; i++) > - { > - ipa_adjusted_param *apm = &(*m_adj_params)[i]; > - if (apm->base_index != index) > - continue; > - if (apm->op == IPA_PARAM_OP_COPY) > - return i; > - if (apm->op == IPA_PARAM_OP_SPLIT > - && apm->unit_offset == unit_offset) > - { > - if (useless_type_conversion_p (apm->type, type)) > - *split_index = i; > - else > - *split_index = -1; > - return -1; > - } > - } > - > - *split_index = -1; > - return -1; > -} > - > /* Return the original index for the given new parameter index. Return a > negative number if not available. */ > > @@ -1020,6 +987,21 @@ ipa_param_adjustments::debug () > dump (stderr); > } > > +/* Register a REPLACEMENT for accesses to BASE at UNIT_OFFSET. */ > + > +void > +ipa_param_body_adjustments::register_replacement (tree base, > + unsigned unit_offset, > + tree replacement) > +{ > + ipa_param_body_replacement psr; > + psr.base = base; > + psr.repl = replacement; > + psr.dummy = NULL_TREE; > + psr.unit_offset = unit_offset; > + m_replacements.safe_push (psr); > +} > + > /* Register that REPLACEMENT should replace parameter described in APM. */ > > void > @@ -1029,12 +1011,8 @@ ipa_param_body_adjustments::register_replacement (ipa_adjusted_param *apm, > gcc_checking_assert (apm->op == IPA_PARAM_OP_SPLIT > || apm->op == IPA_PARAM_OP_NEW); > gcc_checking_assert (!apm->prev_clone_adjustment); > - ipa_param_body_replacement psr; > - psr.base = m_oparms[apm->prev_clone_index]; > - psr.repl = replacement; > - psr.dummy = NULL_TREE; > - psr.unit_offset = apm->unit_offset; > - m_replacements.safe_push (psr); > + register_replacement (m_oparms[apm->prev_clone_index], apm->unit_offset, > + replacement); > } > > /* Copy or not, as appropriate given m_id and decl context, a pre-existing > @@ -1386,23 +1364,73 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, > gcc_unreachable (); > } > > + auto_vec <int, 16> index_mapping; > + bool need_remap = false; > + if (m_id) > + { > + clone_info *cinfo = clone_info::get (m_id->src_node); > + if (cinfo && cinfo->param_adjustments) > + { > + cinfo->param_adjustments->get_updated_indices (&index_mapping); > + need_remap = true; > + } > + > + if (ipcp_transformation *ipcp_ts > + = ipcp_get_transformation_summary (m_id->src_node)) > + { > + for (const ipa_argagg_value &av : ipcp_ts->m_agg_values) > + { > + int parm_num = av.index; > + > + if (need_remap) > + { > + /* FIXME: We cannot handle the situation when IPA-CP > + identified that a parameter is a pointer to a global > + variable and at the same time the variable has some known > + constant contents (PR 107640). The best place to make > + sure we don't drop such constants on the floor probably is > + not here, but we have to make sure that it does not > + confuse the remapping. */ > + if (parm_num >= (int) index_mapping.length ()) > + continue; > + parm_num = index_mapping[parm_num]; > + if (parm_num < 0) > + continue; > + } > + > + if (!kept[parm_num]) > + { > + /* IPA-CP has detected an aggregate constant in a parameter > + that will not be kept, which means that IPA-SRA would have > + split it if there wasn't a constant. Because we are about > + to remove the original, this is the last chance where we > + can substitute the uses with a constant (for values passed > + by reference) or do the split but initialize the > + replacement with a constant (for split aggregates passed > + by value). */ > + > + tree repl; > + if (av.by_ref) > + repl = av.value; > + else > + { > + repl = create_tmp_var (TREE_TYPE (av.value), > + "removed_ipa_cp"); > + gimple *init_stmt = gimple_build_assign (repl, av.value); > + m_split_agg_csts_inits.safe_push (init_stmt); > + } > + register_replacement (m_oparms[parm_num], av.unit_offset, > + repl); > + split[parm_num] = true; > + } > + } > + } > + } > + > if (tree_map) > { > /* Do not treat parameters which were replaced with a constant as > completely vanished. */ > - auto_vec <int, 16> index_mapping; > - bool need_remap = false; > - > - if (m_id) > - { > - clone_info *cinfo = clone_info::get (m_id->src_node); > - if (cinfo && cinfo->param_adjustments) > - { > - cinfo->param_adjustments->get_updated_indices (&index_mapping); > - need_remap = true; > - } > - } > - > for (unsigned i = 0; i < tree_map->length (); i++) > { > int parm_num = (*tree_map)[i]->parm_num; > @@ -1473,8 +1501,9 @@ ipa_param_body_adjustments > : m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (), > m_dead_stmts (), m_dead_ssas (), m_dead_ssa_debug_equiv (), > m_dead_stmt_debug_equiv (), m_fndecl (fndecl), m_id (NULL), m_oparms (), > - m_new_decls (), m_new_types (), m_replacements (), m_removed_decls (), > - m_removed_map (), m_method2func (false) > + m_new_decls (), m_new_types (), m_replacements (), > + m_split_agg_csts_inits (), m_removed_decls (), m_removed_map (), > + m_method2func (false) > { > common_initialization (fndecl, NULL, NULL); > } > @@ -1491,7 +1520,8 @@ ipa_param_body_adjustments > m_reset_debug_decls (), m_dead_stmts (), m_dead_ssas (), > m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (), m_fndecl (fndecl), > m_id (NULL), m_oparms (), m_new_decls (), m_new_types (), m_replacements (), > - m_removed_decls (), m_removed_map (), m_method2func (false) > + m_split_agg_csts_inits (), m_removed_decls (), m_removed_map (), > + m_method2func (false) > { > common_initialization (fndecl, NULL, NULL); > } > @@ -1514,7 +1544,8 @@ ipa_param_body_adjustments > m_reset_debug_decls (), m_dead_stmts (), m_dead_ssas (), > m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (), m_fndecl (fndecl), > m_id (id), m_oparms (), m_new_decls (), m_new_types (), m_replacements (), > - m_removed_decls (), m_removed_map (), m_method2func (false) > + m_split_agg_csts_inits (), m_removed_decls (), m_removed_map (), > + m_method2func (false) > { > common_initialization (old_fndecl, vars, tree_map); > } > @@ -2383,6 +2414,16 @@ ipa_param_body_adjustments::perform_cfun_body_modifications () > } > > > +/* If there are any initialization statements that need to be emitted into > + the basic block BB right at ther start of the new function, do so. */ > +void > +ipa_param_body_adjustments::append_init_stmts (basic_block bb) > +{ > + gimple_stmt_iterator si = gsi_last_bb (bb); > + while (!m_split_agg_csts_inits.is_empty ()) > + gsi_insert_after (&si, m_split_agg_csts_inits.pop (), GSI_NEW_STMT); > +} > + > /* Deallocate summaries which otherwise stay alive until the end of > compilation. */ > > diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h > index e5654f4ff70..e20d34918b3 100644 > --- a/gcc/ipa-param-manipulation.h > +++ b/gcc/ipa-param-manipulation.h > @@ -236,13 +236,6 @@ public: > void get_surviving_params (vec<bool> *surviving_params); > /* Fill a vector with new indices of surviving original parameters. */ > void get_updated_indices (vec<int> *new_indices); > - /* If a parameter with original INDEX has survived intact, return its new > - index. Otherwise return -1. In that case, if it has been split and there > - is a new parameter representing a portion at UNIT_OFFSET for which a value > - of a TYPE can be substituted, store its new index into SPLIT_INDEX, > - otherwise store -1 there. */ > - int get_updated_index_or_split (int index, unsigned unit_offset, tree type, > - int *split_index); > /* Return the original index for the given new parameter index. Return a > negative number if not available. */ > int get_original_index (int newidx); > @@ -321,6 +314,8 @@ public: > > /* Change the PARM_DECLs. */ > void modify_formal_parameters (); > + /* Register a REPLACEMENT for accesses to BASE at UNIT_OFFSET. */ > + void register_replacement (tree base, unsigned unit_offset, tree replacement); > /* Register a replacement decl for the transformation done in APM. */ > void register_replacement (ipa_adjusted_param *apm, tree replacement); > /* Lookup a replacement for a given offset within a given parameter. */ > @@ -340,6 +335,10 @@ public: > they are mapped to. */ > void remap_with_debug_expressions (tree *t); > > + /* If there are any initialization statements that need to be emitted into > + the basic block BB right at ther start of the new function, do so. */ > + void append_init_stmts (basic_block bb); > + > /* Pointers to data structures defining how the function should be > modified. */ > vec<ipa_adjusted_param, va_gc> *m_adj_params; > @@ -405,6 +404,12 @@ private: > > auto_vec<ipa_param_body_replacement, 16> m_replacements; > > + /* List of initialization assignments to be put at the beginning of the > + cloned function to deal with split aggregates which however have known > + constant value and so their PARM_DECL disappears. */ > + > + auto_vec<gimple *, 8> m_split_agg_csts_inits; > + > /* Vector for remapping SSA_BASES from old parameter declarations that are > being removed as a part of the transformation. Before a new VAR_DECL is > created, it holds the old PARM_DECL, once the variable is built it is > diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc > index fcadf64ead7..08c7f97efb9 100644 > --- a/gcc/ipa-prop.cc > +++ b/gcc/ipa-prop.cc > @@ -5518,29 +5518,21 @@ ipcp_read_transformation_summaries (void) > } > } > > -/* Adjust the aggregate replacements in TS to reflect parameters skipped in > - NODE but also if any parameter was IPA-SRAed into a scalar go ahead with > - substitution of the default_definitions of that new param with the > - appropriate constant. > +/* Adjust the aggregate replacements in TS to reflect any parameter removals > + which might have already taken place. If after adjustments there are no > + aggregate replacements left, the m_agg_values will be set to NULL. In other > + cases, it may be shrunk. */ > > - If after adjustments there are no aggregate replacements left, the > - m_agg_values will be set to NULL. In other cases, it may be shrunk. > - > - Return true if any values were already substituted for scalarized parameters > - and update_cfg shuld be run after replace_uses_by. */ > - > -static bool > -adjust_agg_replacement_values (cgraph_node *node, > - ipcp_transformation *ts, > - const vec<ipa_param_descriptor, va_gc> > - &descriptors) > +static void > +adjust_agg_replacement_values (cgraph_node *node, ipcp_transformation *ts) > { > clone_info *cinfo = clone_info::get (node); > if (!cinfo || !cinfo->param_adjustments) > - return false; > + return; > > + auto_vec<int, 16> new_indices; > + cinfo->param_adjustments->get_updated_indices (&new_indices); > bool removed_item = false; > - bool done_replacement = false; > unsigned dst_index = 0; > unsigned count = ts->m_agg_values->length (); > for (unsigned i = 0; i < count; i++) > @@ -5548,13 +5540,10 @@ adjust_agg_replacement_values (cgraph_node *node, > ipa_argagg_value *v = &(*ts->m_agg_values)[i]; > gcc_checking_assert (v->index >= 0); > > - tree cst_type = TREE_TYPE (v->value); > - int split_idx; > - int new_idx > - = cinfo->param_adjustments->get_updated_index_or_split (v->index, > - v->unit_offset, > - cst_type, > - &split_idx); > + int new_idx = -1; > + if ((unsigned) v->index < new_indices.length ()) > + new_idx = new_indices[v->index]; > + > if (new_idx >= 0) > { > v->index = new_idx; > @@ -5563,19 +5552,7 @@ adjust_agg_replacement_values (cgraph_node *node, > dst_index++; > } > else > - { > - removed_item = true; > - if (split_idx >= 0) > - { > - tree parm = ipa_get_param (descriptors, split_idx); > - tree ddef = ssa_default_def (cfun, parm); > - if (ddef) > - { > - replace_uses_by (ddef, v->value); > - done_replacement = true; > - } > - } > - } > + removed_item = true; > } > > if (dst_index == 0) > @@ -5586,7 +5563,7 @@ adjust_agg_replacement_values (cgraph_node *node, > else if (removed_item) > ts->m_agg_values->truncate (dst_index); > > - return done_replacement; > + return; > } > > /* Dominator walker driving the ipcp modification phase. */ > @@ -5955,7 +5932,6 @@ ipcp_update_vr (struct cgraph_node *node) > unsigned int > ipcp_transform_function (struct cgraph_node *node) > { > - vec<ipa_param_descriptor, va_gc> *descriptors = NULL; > struct ipa_func_body_info fbi; > int param_count; > > @@ -5974,18 +5950,13 @@ ipcp_transform_function (struct cgraph_node *node) > param_count = count_formal_params (node->decl); > if (param_count == 0) > return 0; > - vec_safe_grow_cleared (descriptors, param_count, true); > - ipa_populate_param_decls (node, *descriptors); > > - bool cfg_changed = adjust_agg_replacement_values (node, ts, *descriptors); > + adjust_agg_replacement_values (node, ts); > if (vec_safe_is_empty (ts->m_agg_values)) > { > - vec_free (descriptors); > if (dump_file) > fprintf (dump_file, " All affected aggregate parameters were either " > "removed or converted into scalars, phase done.\n"); > - if (cfg_changed) > - delete_unreachable_blocks_update_callgraph (node, false); > return 0; > } > if (dump_file) > @@ -6002,12 +5973,15 @@ ipcp_transform_function (struct cgraph_node *node) > fbi.param_count = param_count; > fbi.aa_walk_budget = opt_for_fn (node->decl, param_ipa_max_aa_steps); > > + vec<ipa_param_descriptor, va_gc> *descriptors = NULL; > + vec_safe_grow_cleared (descriptors, param_count, true); > + ipa_populate_param_decls (node, *descriptors); > bool modified_mem_access = false; > calculate_dominance_info (CDI_DOMINATORS); > ipcp_modif_dom_walker walker (&fbi, descriptors, ts, &modified_mem_access); > walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun)); > free_dominance_info (CDI_DOMINATORS); > - cfg_changed |= walker.cleanup_eh (); > + bool cfg_changed = walker.cleanup_eh (); > > int i; > struct ipa_bb_info *bi; > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc > index 718201d27fa..0f137e810fe 100644 > --- a/gcc/ipa-sra.cc > +++ b/gcc/ipa-sra.cc > @@ -84,6 +84,7 @@ along with GCC; see the file COPYING3. If not see > #include "internal-fn.h" > #include "symtab-clones.h" > #include "attribs.h" > +#include "ipa-prop.h" > > static void ipa_sra_summarize_function (cgraph_node *); > > @@ -3605,13 +3606,16 @@ retval_used_p (cgraph_node *node, void *) > /* Push into NEW_PARAMS all required parameter adjustment entries to copy or > modify parameter which originally had index BASE_INDEX, in the adjustment > vector of parent clone (if any) had PREV_CLONE_INDEX and was described by > - PREV_ADJUSTMENT. If the parent clone is the original function, > - PREV_ADJUSTMENT is NULL and PREV_CLONE_INDEX is equal to BASE_INDEX. */ > + PREV_ADJUSTMENT. If IPA-CP has created a transformation summary for the > + original node, it needs to be passed in IPCP_TS, otherwise it should be > + NULL. If the parent clone is the original function, PREV_ADJUSTMENT is NULL > + and PREV_CLONE_INDEX is equal to BASE_INDEX. */ > > static void > push_param_adjustments_for_index (isra_func_summary *ifs, unsigned base_index, > unsigned prev_clone_index, > ipa_adjusted_param *prev_adjustment, > + ipcp_transformation *ipcp_ts, > vec<ipa_adjusted_param, va_gc> **new_params) > { > isra_param_desc *desc = &(*ifs->m_parameters)[base_index]; > @@ -3652,6 +3656,23 @@ push_param_adjustments_for_index (isra_func_summary *ifs, unsigned base_index, > param_access *pa = (*desc->accesses)[j]; > if (!pa->certain) > continue; > + > + if (ipcp_ts) > + { > + ipa_argagg_value_list avl (ipcp_ts); > + tree value = avl.get_value (base_index, pa->unit_offset); > + if (value > + && (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (value))) / BITS_PER_UNIT > + == pa->unit_size)) > + { > + if (dump_file) > + fprintf (dump_file, " - omitting component at byte " > + "offset %u which is known to have a constant value\n ", > + pa->unit_offset); > + continue; > + } > + } > + > if (dump_file) > fprintf (dump_file, " - component at byte offset %u, " > "size %u\n", pa->unit_offset, pa->unit_size); > @@ -3732,6 +3753,7 @@ process_isra_node_results (cgraph_node *node, > fprintf (dump_file, " Will remove return value.\n"); > } > > + ipcp_transformation *ipcp_ts = ipcp_get_transformation_summary (node); > vec<ipa_adjusted_param, va_gc> *new_params = NULL; > if (ipa_param_adjustments *old_adjustments > = cinfo ? cinfo->param_adjustments : NULL) > @@ -3741,12 +3763,12 @@ process_isra_node_results (cgraph_node *node, > { > ipa_adjusted_param *old_adj = &(*old_adjustments->m_adj_params)[i]; > push_param_adjustments_for_index (ifs, old_adj->base_index, i, > - old_adj, &new_params); > + old_adj, ipcp_ts, &new_params); > } > } > else > for (unsigned i = 0; i < param_count; i++) > - push_param_adjustments_for_index (ifs, i, i, NULL, &new_params); > + push_param_adjustments_for_index (ifs, i, i, NULL, ipcp_ts, &new_params); > > ipa_param_adjustments *new_adjustments > = (new (ggc_alloc <ipa_param_adjustments> ()) > diff --git a/gcc/testsuite/gcc.dg/ipa/pr107640-2.c b/gcc/testsuite/gcc.dg/ipa/pr107640-2.c > new file mode 100644 > index 00000000000..94cbe02860d > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr107640-2.c > @@ -0,0 +1,50 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-optimized-slim" } */ > + > +struct S > +{ > + int a, b, c; > +}; > + > +int ellide (int c); > +volatile short gi; > + > +void __attribute__((noipa)) > +consume_s (struct S *p) > +{ > + gi = p->a; > +} > + > +static void __attribute__((noinline)) > +foo (struct S *p, short *r) > +{ > + gi = *r; > + if (!__builtin_constant_p (p->b)) > + ellide (1); > + consume_s (p); > +} > + > +static void __attribute__((noinline)) > +bar (short *r, struct S *p) > +{ > + gi = *r; > + if (!__builtin_constant_p (p->c)) > + ellide (2); > + consume_s (p); > +} > + > +struct S gs; > + > +int main (int argc, char *argv[]) > +{ > + short i = 42; > + gs.a = 10; > + gs.b = 20; > + foo (&gs, &i); > + gs.b = 30; > + gs.c = 40; > + bar (&i, &gs); > + return 0; > +} > + > +/* { dg-final { scan-tree-dump-not "ellide" "optimized" { xfail *-*-* } } } */ > diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc > index 8091ba8f13b..15a1a389493 100644 > --- a/gcc/tree-inline.cc > +++ b/gcc/tree-inline.cc > @@ -6377,6 +6377,8 @@ tree_function_versioning (tree old_decl, tree new_decl, > bb = split_edge (single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun))); > while (init_stmts.length ()) > insert_init_stmt (&id, bb, init_stmts.pop ()); > + if (param_body_adjs) > + param_body_adjs->append_init_stmts (bb); > update_clone_info (&id); > > /* Remap the nonlocal_goto_save_area, if any. */ > -- > 2.38.1 >
diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc index cee0e23f946..e92cfc0b6d5 100644 --- a/gcc/ipa-param-manipulation.cc +++ b/gcc/ipa-param-manipulation.cc @@ -46,7 +46,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-phinodes.h" #include "cfgexpand.h" #include "attribs.h" - +#include "ipa-prop.h" /* Actual prefixes of different newly synthetized parameters. Keep in sync with IPA_PARAM_PREFIX_* defines. */ @@ -449,39 +449,6 @@ ipa_param_adjustments::get_updated_indices (vec<int> *new_indices) } } -/* If a parameter with original INDEX has survived intact, return its new - index. Otherwise return -1. In that case, if it has been split and there - is a new parameter representing a portion at unit OFFSET for which a value - of a TYPE can be substituted, store its new index into SPLIT_INDEX, - otherwise store -1 there. */ -int -ipa_param_adjustments::get_updated_index_or_split (int index, - unsigned unit_offset, - tree type, int *split_index) -{ - unsigned adj_len = vec_safe_length (m_adj_params); - for (unsigned i = 0; i < adj_len ; i++) - { - ipa_adjusted_param *apm = &(*m_adj_params)[i]; - if (apm->base_index != index) - continue; - if (apm->op == IPA_PARAM_OP_COPY) - return i; - if (apm->op == IPA_PARAM_OP_SPLIT - && apm->unit_offset == unit_offset) - { - if (useless_type_conversion_p (apm->type, type)) - *split_index = i; - else - *split_index = -1; - return -1; - } - } - - *split_index = -1; - return -1; -} - /* Return the original index for the given new parameter index. Return a negative number if not available. */ @@ -1020,6 +987,21 @@ ipa_param_adjustments::debug () dump (stderr); } +/* Register a REPLACEMENT for accesses to BASE at UNIT_OFFSET. */ + +void +ipa_param_body_adjustments::register_replacement (tree base, + unsigned unit_offset, + tree replacement) +{ + ipa_param_body_replacement psr; + psr.base = base; + psr.repl = replacement; + psr.dummy = NULL_TREE; + psr.unit_offset = unit_offset; + m_replacements.safe_push (psr); +} + /* Register that REPLACEMENT should replace parameter described in APM. */ void @@ -1029,12 +1011,8 @@ ipa_param_body_adjustments::register_replacement (ipa_adjusted_param *apm, gcc_checking_assert (apm->op == IPA_PARAM_OP_SPLIT || apm->op == IPA_PARAM_OP_NEW); gcc_checking_assert (!apm->prev_clone_adjustment); - ipa_param_body_replacement psr; - psr.base = m_oparms[apm->prev_clone_index]; - psr.repl = replacement; - psr.dummy = NULL_TREE; - psr.unit_offset = apm->unit_offset; - m_replacements.safe_push (psr); + register_replacement (m_oparms[apm->prev_clone_index], apm->unit_offset, + replacement); } /* Copy or not, as appropriate given m_id and decl context, a pre-existing @@ -1386,23 +1364,73 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, gcc_unreachable (); } + auto_vec <int, 16> index_mapping; + bool need_remap = false; + if (m_id) + { + clone_info *cinfo = clone_info::get (m_id->src_node); + if (cinfo && cinfo->param_adjustments) + { + cinfo->param_adjustments->get_updated_indices (&index_mapping); + need_remap = true; + } + + if (ipcp_transformation *ipcp_ts + = ipcp_get_transformation_summary (m_id->src_node)) + { + for (const ipa_argagg_value &av : ipcp_ts->m_agg_values) + { + int parm_num = av.index; + + if (need_remap) + { + /* FIXME: We cannot handle the situation when IPA-CP + identified that a parameter is a pointer to a global + variable and at the same time the variable has some known + constant contents (PR 107640). The best place to make + sure we don't drop such constants on the floor probably is + not here, but we have to make sure that it does not + confuse the remapping. */ + if (parm_num >= (int) index_mapping.length ()) + continue; + parm_num = index_mapping[parm_num]; + if (parm_num < 0) + continue; + } + + if (!kept[parm_num]) + { + /* IPA-CP has detected an aggregate constant in a parameter + that will not be kept, which means that IPA-SRA would have + split it if there wasn't a constant. Because we are about + to remove the original, this is the last chance where we + can substitute the uses with a constant (for values passed + by reference) or do the split but initialize the + replacement with a constant (for split aggregates passed + by value). */ + + tree repl; + if (av.by_ref) + repl = av.value; + else + { + repl = create_tmp_var (TREE_TYPE (av.value), + "removed_ipa_cp"); + gimple *init_stmt = gimple_build_assign (repl, av.value); + m_split_agg_csts_inits.safe_push (init_stmt); + } + register_replacement (m_oparms[parm_num], av.unit_offset, + repl); + split[parm_num] = true; + } + } + } + } + if (tree_map) { /* Do not treat parameters which were replaced with a constant as completely vanished. */ - auto_vec <int, 16> index_mapping; - bool need_remap = false; - - if (m_id) - { - clone_info *cinfo = clone_info::get (m_id->src_node); - if (cinfo && cinfo->param_adjustments) - { - cinfo->param_adjustments->get_updated_indices (&index_mapping); - need_remap = true; - } - } - for (unsigned i = 0; i < tree_map->length (); i++) { int parm_num = (*tree_map)[i]->parm_num; @@ -1473,8 +1501,9 @@ ipa_param_body_adjustments : m_adj_params (adj_params), m_adjustments (NULL), m_reset_debug_decls (), m_dead_stmts (), m_dead_ssas (), m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (), m_fndecl (fndecl), m_id (NULL), m_oparms (), - m_new_decls (), m_new_types (), m_replacements (), m_removed_decls (), - m_removed_map (), m_method2func (false) + m_new_decls (), m_new_types (), m_replacements (), + m_split_agg_csts_inits (), m_removed_decls (), m_removed_map (), + m_method2func (false) { common_initialization (fndecl, NULL, NULL); } @@ -1491,7 +1520,8 @@ ipa_param_body_adjustments m_reset_debug_decls (), m_dead_stmts (), m_dead_ssas (), m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (), m_fndecl (fndecl), m_id (NULL), m_oparms (), m_new_decls (), m_new_types (), m_replacements (), - m_removed_decls (), m_removed_map (), m_method2func (false) + m_split_agg_csts_inits (), m_removed_decls (), m_removed_map (), + m_method2func (false) { common_initialization (fndecl, NULL, NULL); } @@ -1514,7 +1544,8 @@ ipa_param_body_adjustments m_reset_debug_decls (), m_dead_stmts (), m_dead_ssas (), m_dead_ssa_debug_equiv (), m_dead_stmt_debug_equiv (), m_fndecl (fndecl), m_id (id), m_oparms (), m_new_decls (), m_new_types (), m_replacements (), - m_removed_decls (), m_removed_map (), m_method2func (false) + m_split_agg_csts_inits (), m_removed_decls (), m_removed_map (), + m_method2func (false) { common_initialization (old_fndecl, vars, tree_map); } @@ -2383,6 +2414,16 @@ ipa_param_body_adjustments::perform_cfun_body_modifications () } +/* If there are any initialization statements that need to be emitted into + the basic block BB right at ther start of the new function, do so. */ +void +ipa_param_body_adjustments::append_init_stmts (basic_block bb) +{ + gimple_stmt_iterator si = gsi_last_bb (bb); + while (!m_split_agg_csts_inits.is_empty ()) + gsi_insert_after (&si, m_split_agg_csts_inits.pop (), GSI_NEW_STMT); +} + /* Deallocate summaries which otherwise stay alive until the end of compilation. */ diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h index e5654f4ff70..e20d34918b3 100644 --- a/gcc/ipa-param-manipulation.h +++ b/gcc/ipa-param-manipulation.h @@ -236,13 +236,6 @@ public: void get_surviving_params (vec<bool> *surviving_params); /* Fill a vector with new indices of surviving original parameters. */ void get_updated_indices (vec<int> *new_indices); - /* If a parameter with original INDEX has survived intact, return its new - index. Otherwise return -1. In that case, if it has been split and there - is a new parameter representing a portion at UNIT_OFFSET for which a value - of a TYPE can be substituted, store its new index into SPLIT_INDEX, - otherwise store -1 there. */ - int get_updated_index_or_split (int index, unsigned unit_offset, tree type, - int *split_index); /* Return the original index for the given new parameter index. Return a negative number if not available. */ int get_original_index (int newidx); @@ -321,6 +314,8 @@ public: /* Change the PARM_DECLs. */ void modify_formal_parameters (); + /* Register a REPLACEMENT for accesses to BASE at UNIT_OFFSET. */ + void register_replacement (tree base, unsigned unit_offset, tree replacement); /* Register a replacement decl for the transformation done in APM. */ void register_replacement (ipa_adjusted_param *apm, tree replacement); /* Lookup a replacement for a given offset within a given parameter. */ @@ -340,6 +335,10 @@ public: they are mapped to. */ void remap_with_debug_expressions (tree *t); + /* If there are any initialization statements that need to be emitted into + the basic block BB right at ther start of the new function, do so. */ + void append_init_stmts (basic_block bb); + /* Pointers to data structures defining how the function should be modified. */ vec<ipa_adjusted_param, va_gc> *m_adj_params; @@ -405,6 +404,12 @@ private: auto_vec<ipa_param_body_replacement, 16> m_replacements; + /* List of initialization assignments to be put at the beginning of the + cloned function to deal with split aggregates which however have known + constant value and so their PARM_DECL disappears. */ + + auto_vec<gimple *, 8> m_split_agg_csts_inits; + /* Vector for remapping SSA_BASES from old parameter declarations that are being removed as a part of the transformation. Before a new VAR_DECL is created, it holds the old PARM_DECL, once the variable is built it is diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc index fcadf64ead7..08c7f97efb9 100644 --- a/gcc/ipa-prop.cc +++ b/gcc/ipa-prop.cc @@ -5518,29 +5518,21 @@ ipcp_read_transformation_summaries (void) } } -/* Adjust the aggregate replacements in TS to reflect parameters skipped in - NODE but also if any parameter was IPA-SRAed into a scalar go ahead with - substitution of the default_definitions of that new param with the - appropriate constant. +/* Adjust the aggregate replacements in TS to reflect any parameter removals + which might have already taken place. If after adjustments there are no + aggregate replacements left, the m_agg_values will be set to NULL. In other + cases, it may be shrunk. */ - If after adjustments there are no aggregate replacements left, the - m_agg_values will be set to NULL. In other cases, it may be shrunk. - - Return true if any values were already substituted for scalarized parameters - and update_cfg shuld be run after replace_uses_by. */ - -static bool -adjust_agg_replacement_values (cgraph_node *node, - ipcp_transformation *ts, - const vec<ipa_param_descriptor, va_gc> - &descriptors) +static void +adjust_agg_replacement_values (cgraph_node *node, ipcp_transformation *ts) { clone_info *cinfo = clone_info::get (node); if (!cinfo || !cinfo->param_adjustments) - return false; + return; + auto_vec<int, 16> new_indices; + cinfo->param_adjustments->get_updated_indices (&new_indices); bool removed_item = false; - bool done_replacement = false; unsigned dst_index = 0; unsigned count = ts->m_agg_values->length (); for (unsigned i = 0; i < count; i++) @@ -5548,13 +5540,10 @@ adjust_agg_replacement_values (cgraph_node *node, ipa_argagg_value *v = &(*ts->m_agg_values)[i]; gcc_checking_assert (v->index >= 0); - tree cst_type = TREE_TYPE (v->value); - int split_idx; - int new_idx - = cinfo->param_adjustments->get_updated_index_or_split (v->index, - v->unit_offset, - cst_type, - &split_idx); + int new_idx = -1; + if ((unsigned) v->index < new_indices.length ()) + new_idx = new_indices[v->index]; + if (new_idx >= 0) { v->index = new_idx; @@ -5563,19 +5552,7 @@ adjust_agg_replacement_values (cgraph_node *node, dst_index++; } else - { - removed_item = true; - if (split_idx >= 0) - { - tree parm = ipa_get_param (descriptors, split_idx); - tree ddef = ssa_default_def (cfun, parm); - if (ddef) - { - replace_uses_by (ddef, v->value); - done_replacement = true; - } - } - } + removed_item = true; } if (dst_index == 0) @@ -5586,7 +5563,7 @@ adjust_agg_replacement_values (cgraph_node *node, else if (removed_item) ts->m_agg_values->truncate (dst_index); - return done_replacement; + return; } /* Dominator walker driving the ipcp modification phase. */ @@ -5955,7 +5932,6 @@ ipcp_update_vr (struct cgraph_node *node) unsigned int ipcp_transform_function (struct cgraph_node *node) { - vec<ipa_param_descriptor, va_gc> *descriptors = NULL; struct ipa_func_body_info fbi; int param_count; @@ -5974,18 +5950,13 @@ ipcp_transform_function (struct cgraph_node *node) param_count = count_formal_params (node->decl); if (param_count == 0) return 0; - vec_safe_grow_cleared (descriptors, param_count, true); - ipa_populate_param_decls (node, *descriptors); - bool cfg_changed = adjust_agg_replacement_values (node, ts, *descriptors); + adjust_agg_replacement_values (node, ts); if (vec_safe_is_empty (ts->m_agg_values)) { - vec_free (descriptors); if (dump_file) fprintf (dump_file, " All affected aggregate parameters were either " "removed or converted into scalars, phase done.\n"); - if (cfg_changed) - delete_unreachable_blocks_update_callgraph (node, false); return 0; } if (dump_file) @@ -6002,12 +5973,15 @@ ipcp_transform_function (struct cgraph_node *node) fbi.param_count = param_count; fbi.aa_walk_budget = opt_for_fn (node->decl, param_ipa_max_aa_steps); + vec<ipa_param_descriptor, va_gc> *descriptors = NULL; + vec_safe_grow_cleared (descriptors, param_count, true); + ipa_populate_param_decls (node, *descriptors); bool modified_mem_access = false; calculate_dominance_info (CDI_DOMINATORS); ipcp_modif_dom_walker walker (&fbi, descriptors, ts, &modified_mem_access); walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun)); free_dominance_info (CDI_DOMINATORS); - cfg_changed |= walker.cleanup_eh (); + bool cfg_changed = walker.cleanup_eh (); int i; struct ipa_bb_info *bi; diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index 718201d27fa..0f137e810fe 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -84,6 +84,7 @@ along with GCC; see the file COPYING3. If not see #include "internal-fn.h" #include "symtab-clones.h" #include "attribs.h" +#include "ipa-prop.h" static void ipa_sra_summarize_function (cgraph_node *); @@ -3605,13 +3606,16 @@ retval_used_p (cgraph_node *node, void *) /* Push into NEW_PARAMS all required parameter adjustment entries to copy or modify parameter which originally had index BASE_INDEX, in the adjustment vector of parent clone (if any) had PREV_CLONE_INDEX and was described by - PREV_ADJUSTMENT. If the parent clone is the original function, - PREV_ADJUSTMENT is NULL and PREV_CLONE_INDEX is equal to BASE_INDEX. */ + PREV_ADJUSTMENT. If IPA-CP has created a transformation summary for the + original node, it needs to be passed in IPCP_TS, otherwise it should be + NULL. If the parent clone is the original function, PREV_ADJUSTMENT is NULL + and PREV_CLONE_INDEX is equal to BASE_INDEX. */ static void push_param_adjustments_for_index (isra_func_summary *ifs, unsigned base_index, unsigned prev_clone_index, ipa_adjusted_param *prev_adjustment, + ipcp_transformation *ipcp_ts, vec<ipa_adjusted_param, va_gc> **new_params) { isra_param_desc *desc = &(*ifs->m_parameters)[base_index]; @@ -3652,6 +3656,23 @@ push_param_adjustments_for_index (isra_func_summary *ifs, unsigned base_index, param_access *pa = (*desc->accesses)[j]; if (!pa->certain) continue; + + if (ipcp_ts) + { + ipa_argagg_value_list avl (ipcp_ts); + tree value = avl.get_value (base_index, pa->unit_offset); + if (value + && (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (value))) / BITS_PER_UNIT + == pa->unit_size)) + { + if (dump_file) + fprintf (dump_file, " - omitting component at byte " + "offset %u which is known to have a constant value\n ", + pa->unit_offset); + continue; + } + } + if (dump_file) fprintf (dump_file, " - component at byte offset %u, " "size %u\n", pa->unit_offset, pa->unit_size); @@ -3732,6 +3753,7 @@ process_isra_node_results (cgraph_node *node, fprintf (dump_file, " Will remove return value.\n"); } + ipcp_transformation *ipcp_ts = ipcp_get_transformation_summary (node); vec<ipa_adjusted_param, va_gc> *new_params = NULL; if (ipa_param_adjustments *old_adjustments = cinfo ? cinfo->param_adjustments : NULL) @@ -3741,12 +3763,12 @@ process_isra_node_results (cgraph_node *node, { ipa_adjusted_param *old_adj = &(*old_adjustments->m_adj_params)[i]; push_param_adjustments_for_index (ifs, old_adj->base_index, i, - old_adj, &new_params); + old_adj, ipcp_ts, &new_params); } } else for (unsigned i = 0; i < param_count; i++) - push_param_adjustments_for_index (ifs, i, i, NULL, &new_params); + push_param_adjustments_for_index (ifs, i, i, NULL, ipcp_ts, &new_params); ipa_param_adjustments *new_adjustments = (new (ggc_alloc <ipa_param_adjustments> ()) diff --git a/gcc/testsuite/gcc.dg/ipa/pr107640-2.c b/gcc/testsuite/gcc.dg/ipa/pr107640-2.c new file mode 100644 index 00000000000..94cbe02860d --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr107640-2.c @@ -0,0 +1,50 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized-slim" } */ + +struct S +{ + int a, b, c; +}; + +int ellide (int c); +volatile short gi; + +void __attribute__((noipa)) +consume_s (struct S *p) +{ + gi = p->a; +} + +static void __attribute__((noinline)) +foo (struct S *p, short *r) +{ + gi = *r; + if (!__builtin_constant_p (p->b)) + ellide (1); + consume_s (p); +} + +static void __attribute__((noinline)) +bar (short *r, struct S *p) +{ + gi = *r; + if (!__builtin_constant_p (p->c)) + ellide (2); + consume_s (p); +} + +struct S gs; + +int main (int argc, char *argv[]) +{ + short i = 42; + gs.a = 10; + gs.b = 20; + foo (&gs, &i); + gs.b = 30; + gs.c = 40; + bar (&i, &gs); + return 0; +} + +/* { dg-final { scan-tree-dump-not "ellide" "optimized" { xfail *-*-* } } } */ diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index 8091ba8f13b..15a1a389493 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -6377,6 +6377,8 @@ tree_function_versioning (tree old_decl, tree new_decl, bb = split_edge (single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun))); while (init_stmts.length ()) insert_init_stmt (&id, bb, init_stmts.pop ()); + if (param_body_adjs) + param_body_adjs->append_init_stmts (bb); update_clone_info (&id); /* Remap the nonlocal_goto_save_area, if any. */
Hi, I'm re-posting patches which I have posted at the end of stage 1 but which have not passed review yet. I would really like to have at least the previous patch and this one accepted for GCC 13 so that PR 103227 is fixed ina way which does not sometimes leave unused parameters behind. The rest of the patches make IPA-SRA more powerful but if they cannot be reviewed in time, I guess they'll be fine in GCC 14 too. 8<-------------------------------------------------------------------- This is basically a better fix for PR 103227. The one currently in use, rushed in late at stage3, which means that IPA-CP transformation simply does a replacement of default-definition of IPA-SRA-created scalar parameters with a constant, meant that IPA-SRA actually often led to creation of a bunch of unused parameters, which was rather ironic and sub-optimal. This patch rips that old way out and makes sure the clash is resolved at clone-materialization time. What happens is that: 1) IPA-SRA IPA analysis (decision) stage recognizes the clash and does not create a param adjustment entry for such a scalar component. 2) Clone materialization code checks the IPA-CP transformation summary and when it realizes that it is removing a parameter that is a base for a discovered IPA-CP aggregate constant, and: a) the value is passed by reference, it internally records that any load of the value is replaced directly with the known constant value. IPA-SRA will not attempt to split values passed by reference when there is a write to it so we know such a load won't be on a a LHS. b) the value is passed by value, there can be stores to the corresponding bit of the aggregate and so all accesses are replaced with a new decl and an assignment of the constant to this decl is generated at the beginning of the function. The new testcase contains an xfail as the patch does not fix PR 107640 but it is one that ICEs when one is not careful about remapping indices of parameters, so I'd like to have it in testsuite/gcc.gd/ipa/ even now. I don't think that PR 107640 should be attempted through ipa-param-manipulation replacements because the information is not really there any more and we'd either need to do the replacements earlier or dig deep into the clone parent info. Instead, we should record somewhere that at the beginning of the function the bits of the global decl have known values and use that in the value numbering. That way we could one day encode also known constants in globals that do not come through parameters. Bootstrapped and LTO-bootstrapped on x86_64-linux. OK for master? gcc/ChangeLog: 2022-11-11 Martin Jambor <mjambor@suse.cz> PR ipa/103227 * ipa-param-manipulation.h (class ipa_param_adjustments): Removed member function get_updated_index_or_split. (class ipa_param_body_adjustments): New overload of register_replacement, new member function append_init_stmts, new member m_split_agg_csts_inits. * ipa-param-manipulation.cc: Include ipa-prop.h. (ipa_param_adjustments::get_updated_index_or_split): Removed. (ipa_param_body_adjustments::register_replacement): New overload, use it from the older one. (ipa_param_body_adjustments::common_initialization): Added the capability to create replacements for conflicting IPA-CP discovered constants. (ipa_param_body_adjustments::ipa_param_body_adjustments): Construct the new member. (ipa_param_body_adjustments::append_init_stmts): New function. * ipa-sra.cc: Include ipa-prop.h. (push_param_adjustments_for_index): Require IPA-CP transformation summary as a parameter, do not create replacements which are known to have constant values. (process_isra_node_results): Find and pass to the above function the IPA-CP transformation summary. * ipa-prop.cc (adjust_agg_replacement_values): Remove the functionality replacing IPA-SRA created scalar parameters with constants. Simplify, do not require parameter descriptors, do not return anything. (ipcp_transform_function): Simplify now that adjust_agg_replacement_values does not change cfg. Move definition and initialization of descriptors lower. * tree-inline.cc (tree_function_versioning): Call append_init_stmts of param_body_adjs, if there are any. gcc/testsuite/ChangeLog: 2022-11-11 Martin Jambor <mjambor@suse.cz> PR ipa/103227 PR ipa/107640 * gcc.dg/ipa/pr107640-2.c: New test. --- gcc/ipa-param-manipulation.cc | 155 ++++++++++++++++---------- gcc/ipa-param-manipulation.h | 19 ++-- gcc/ipa-prop.cc | 66 ++++------- gcc/ipa-sra.cc | 30 ++++- gcc/testsuite/gcc.dg/ipa/pr107640-2.c | 50 +++++++++ gcc/tree-inline.cc | 2 + 6 files changed, 208 insertions(+), 114 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/ipa/pr107640-2.c