| Message ID | ri6359kimxk.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] ipa-cp: Write transformation summaries of all functions | expand |
> Hi, > > I'm re-posting patches which I have posted at the end of stage 1 but > which have not passed review yet. > > 8<-------------------------------------------------------------------- > > C++ and especially Fortran pass data by references which are not > pointers potentially pointing anywhere and so can be assumed to be > safely dereferencable. This patch teaches IPA-SRA to treat them as > such and avoid the dance we do to prove that we can move loads from > them to the caller. > > When we do not know that a dereference will happen all the time, we > need a heuristics so that we do not force memory accesses that normally > happen only rarely. The patch simply uses the (possibly guessed) > profile and checks whether the (expected) number of loads is at least > half of function invocations invocations. > > Bootstrapped and tested individually when I originally posted it and > now bootstrapped and LTO-bootstrapped and tested as part of the whole > series. OK for master? > > > gcc/ChangeLog: > > 2022-11-11 Martin Jambor <mjambor@suse.cz> > > PR ipa/103585 > * ipa-sra.c (struct gensum_param_access): New field load_count. > (struct gensum_param_desc): New field safe_ref, adjusted comments. > (by_ref_count): Renamed to unsafe_by_ref_count, adjusted all uses. > (dump_gensum_access): Dump the new field. > (dump_gensum_param_descriptor): Likewise. > (create_parameter_descriptors): Set safe_ref field, move setting > by_ref forward. Only increment unsafe_by_ref_count for unsafe > by_ref parameters. > (allocate_access): Initialize new field. > (mark_param_dereference): Adjust indentation. Only add data to > bb_dereferences for unsafe by_ref parameters. > (scan_expr_access): For loads, accumulate BB counts. > (dereference_probable_p): New function. > (check_gensum_access): Fix leading comment, add parameter FUN. > Check cumulative counts of loads for safe by_ref accesses instead > of dereferences. > (process_scan_results): Do not propagate dereference distances for > safe by_ref parameters. Pass fun to check_gensum_access. Safe > by_ref params do not need the postdominance check. > > gcc/testsuite/ChangeLog: > > 2022-11-11 Martin Jambor <mjambor@suse.cz> > > * g++.dg/ipa/ipa-sra-5.C: New test > -/* Perform basic checks on ACCESS to PARM described by DESC and all its > - children, return true if the parameter cannot be split, otherwise return > - true and update *TOTAL_SIZE and *ONLY_CALLS. ENTRY_BB_INDEX must be the > - index of the entry BB in the function of PARM. */ > +/* Return true if the ACCESS loads happen frequently enough in FUN to risk > + moving them to the caller and only pass the result. */ > > static bool > -check_gensum_access (tree parm, gensum_param_desc *desc, > +dereference_probable_p (struct function *fun, gensum_param_access *access) > +{ > + return access->load_count > + >= ENTRY_BLOCK_PTR_FOR_FN (fun)->count.apply_scale (1, 2); We may want to have --param for this. Otherwise the patch is OK. Honza
Hi, On Mon, Dec 12 2022, Jan Hubicka wrote: >> [...] >> gcc/ChangeLog: >> >> 2022-11-11 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/103585 >> * ipa-sra.c (struct gensum_param_access): New field load_count. >> (struct gensum_param_desc): New field safe_ref, adjusted comments. >> (by_ref_count): Renamed to unsafe_by_ref_count, adjusted all uses. >> (dump_gensum_access): Dump the new field. >> (dump_gensum_param_descriptor): Likewise. >> (create_parameter_descriptors): Set safe_ref field, move setting >> by_ref forward. Only increment unsafe_by_ref_count for unsafe >> by_ref parameters. >> (allocate_access): Initialize new field. >> (mark_param_dereference): Adjust indentation. Only add data to >> bb_dereferences for unsafe by_ref parameters. >> (scan_expr_access): For loads, accumulate BB counts. >> (dereference_probable_p): New function. >> (check_gensum_access): Fix leading comment, add parameter FUN. >> Check cumulative counts of loads for safe by_ref accesses instead >> of dereferences. >> (process_scan_results): Do not propagate dereference distances for >> safe by_ref parameters. Pass fun to check_gensum_access. Safe >> by_ref params do not need the postdominance check. >> >> gcc/testsuite/ChangeLog: >> >> 2022-11-11 Martin Jambor <mjambor@suse.cz> >> >> * g++.dg/ipa/ipa-sra-5.C: New test >> -/* Perform basic checks on ACCESS to PARM described by DESC and all its >> - children, return true if the parameter cannot be split, otherwise return >> - true and update *TOTAL_SIZE and *ONLY_CALLS. ENTRY_BB_INDEX must be the >> - index of the entry BB in the function of PARM. */ >> +/* Return true if the ACCESS loads happen frequently enough in FUN to risk >> + moving them to the caller and only pass the result. */ >> >> static bool >> -check_gensum_access (tree parm, gensum_param_desc *desc, >> +dereference_probable_p (struct function *fun, gensum_param_access *access) >> +{ >> + return access->load_count >> + >= ENTRY_BLOCK_PTR_FOR_FN (fun)->count.apply_scale (1, 2); > > We may want to have --param for this. > OK, I have added a percentage-style parameter. I plan to commit the following after the last round of testing (I have already checked the documentation bits with make info and make pdf). Thanks! Martin C++ and especially Fortran pass data by references which are not pointers potentially pointing anywhere and so can be assumed to be safely dereferencable. This patch teaches IPA-SRA to treat them as such and avoid the dance we do to prove that we can move loads from them to the caller. When we do not know that a dereference will happen all the time, we need a heuristics so that we do not force memory accesses that normally happen only rarely. The patch simply uses the (possibly guessed) profile and checks whether the (expected) number of loads is at least half of function invocations invocations - the half is now configurable with a param as requested by Honza. gcc/ChangeLog: 2022-12-13 Martin Jambor <mjambor@suse.cz> PR ipa/103585 * params.opt (ipa-sra-deref-prob-threshold): New parameter. * doc/invoke.texi (ipa-sra-deref-prob-threshold): Document it. * ipa-sra.cc (struct gensum_param_access): New field load_count. (struct gensum_param_desc): New field safe_ref, adjusted comments. (by_ref_count): Renamed to unsafe_by_ref_count, adjusted all uses. (dump_gensum_access): Dump the new field. (dump_gensum_param_descriptor): Likewise. (create_parameter_descriptors): Set safe_ref field, move setting by_ref forward. Only increment unsafe_by_ref_count for unsafe by_ref parameters. (allocate_access): Initialize new field. (mark_param_dereference): Adjust indentation. Only add data to bb_dereferences for unsafe by_ref parameters. (scan_expr_access): For loads, accumulate BB counts. (dereference_probable_p): New function. (check_gensum_access): Fix leading comment, add parameter FUN. Check cumulative counts of loads for safe by_ref accesses instead of dereferences. (process_scan_results): Do not propagate dereference distances for safe by_ref parameters. Pass fun to check_gensum_access. Safe by_ref params do not need the postdominance check. gcc/testsuite/ChangeLog: 2022-11-11 Martin Jambor <mjambor@suse.cz> * g++.dg/ipa/ipa-sra-5.C: New test --- gcc/doc/invoke.texi | 5 ++ gcc/ipa-sra.cc | 101 +++++++++++++++++++-------- gcc/params.opt | 4 ++ gcc/testsuite/g++.dg/ipa/ipa-sra-5.C | 23 ++++++ 4 files changed, 105 insertions(+), 28 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/ipa-sra-5.C diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index cb40b38b73a..8eae914f10e 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -15508,6 +15508,11 @@ the parameter is reserved exclusively for debug insns created by @option{-fvar-tracking-assignments}, but debug insns may get (non-overlapping) uids above it if the reserved range is exhausted. +@item ipa-sra-deref-prob-threshold +IPA-SRA replaces a pointer which is known not be NULL with one or more +new parameters only when the probability (in percent, relative to +function entry) of it being dereferenced is higher than this parameter. + @item ipa-sra-ptr-growth-factor IPA-SRA replaces a pointer to an aggregate with one or more new parameters only when their cumulative size is less or equal to diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index 0f137e810fe..866f52e89e0 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -151,6 +151,8 @@ struct gensum_param_access arguments. */ tree alias_ptr_type; + /* Cumulative count of all loads. */ + profile_count load_count; /* Have there been writes to or reads from this exact location except for as arguments to a function call that can be tracked. */ bool nonarg; @@ -207,8 +209,13 @@ struct gensum_param_desc by reference that is a candidate for being converted to a set of parameters passing those data by value. */ bool split_candidate; - /* Is this a parameter passing stuff by reference? */ + /* Is this a parameter passing stuff by reference (either a pointer or a + source language reference type)? */ bool by_ref; + /* If this parameter passes stuff by reference, can it be safely dereferenced + without performing further checks (for example because it is a + REFERENCE_TYPE)? */ + bool safe_ref; /* The number of this parameter as they are ordered in function decl. */ int param_number; @@ -561,7 +568,7 @@ int aa_walking_limit; accessed in that BB. */ HOST_WIDE_INT *bb_dereferences = NULL; /* How many by-reference parameters there are in the current function. */ -int by_ref_count; +int unsafe_by_ref_count; /* Bitmap of BBs that can cause the function to "stop" progressing by returning, throwing externally, looping infinitely or calling a function @@ -643,6 +650,8 @@ dump_gensum_access (FILE *f, gensum_param_access *access, unsigned indent) print_generic_expr (f, access->type); fprintf (f, ", alias_ptr_type: "); print_generic_expr (f, access->alias_ptr_type); + fprintf (f, ", load_count: "); + access->load_count.dump (f); fprintf (f, ", nonarg: %u, reverse: %u\n", access->nonarg, access->reverse); for (gensum_param_access *ch = access->first_child; ch; @@ -692,7 +701,8 @@ dump_gensum_param_descriptor (FILE *f, gensum_param_desc *desc) return; } if (desc->by_ref) - fprintf (f, " by_ref with %u pass throughs\n", desc->ptr_pt_count); + fprintf (f, " %s by_ref with %u pass throughs\n", + desc->safe_ref ? "safe" : "unsafe", desc->ptr_pt_count); for (gensum_param_access *acc = desc->accesses; acc; acc = acc->next_sibling) dump_gensum_access (f, acc, 2); @@ -1140,6 +1150,8 @@ create_parameter_descriptors (cgraph_node *node, if (POINTER_TYPE_P (type)) { + desc->by_ref = true; + desc->safe_ref = (TREE_CODE (type) == REFERENCE_TYPE); type = TREE_TYPE (type); if (TREE_CODE (type) == FUNCTION_TYPE @@ -1187,7 +1199,6 @@ create_parameter_descriptors (cgraph_node *node, "nonarg uses\n"); continue; } - desc->by_ref = true; } else if (!AGGREGATE_TYPE_P (type)) { @@ -1231,8 +1242,8 @@ create_parameter_descriptors (cgraph_node *node, ret = true; desc->split_candidate = true; - if (desc->by_ref) - desc->deref_index = by_ref_count++; + if (desc->by_ref && !desc->safe_ref) + desc->deref_index = unsafe_by_ref_count++; } return ret; } @@ -1301,6 +1312,7 @@ allocate_access (gensum_param_desc *desc, memset (access, 0, sizeof (*access)); access->offset = offset; access->size = size; + access->load_count = profile_count::zero (); return access; } @@ -1555,15 +1567,16 @@ asm_visit_addr (gimple *, tree op, tree, void *) static void mark_param_dereference (gensum_param_desc *desc, HOST_WIDE_INT dist, - basic_block bb) + basic_block bb) { gcc_assert (desc->by_ref); gcc_checking_assert (desc->split_candidate); - if (bitmap_bit_p (final_bbs, bb->index)) + if (desc->safe_ref + || bitmap_bit_p (final_bbs, bb->index)) return; - int idx = bb->index * by_ref_count + desc->deref_index; + int idx = bb->index * unsafe_by_ref_count + desc->deref_index; if (bb_dereferences[idx] < dist) bb_dereferences[idx] = dist; } @@ -1811,6 +1824,8 @@ scan_expr_access (tree expr, gimple *stmt, isra_scan_context ctx, other. */ access->nonarg = true; } + else if (ctx == ISRA_CTX_LOAD && bb->count.initialized_p ()) + access->load_count += bb->count; if (!access->type) { @@ -2092,9 +2107,9 @@ dump_dereferences_table (FILE *f, struct function *fun, const char *str) if (bb != EXIT_BLOCK_PTR_FOR_FN (fun)) { int i; - for (i = 0; i < by_ref_count; i++) + for (i = 0; i < unsafe_by_ref_count; i++) { - int idx = bb->index * by_ref_count + i; + int idx = bb->index * unsafe_by_ref_count + i; fprintf (f, " %4" HOST_WIDE_INT_PRINT "d", bb_dereferences[idx]); } } @@ -2139,15 +2154,15 @@ propagate_dereference_distances (struct function *fun) if (bitmap_bit_p (final_bbs, bb->index)) continue; - for (i = 0; i < by_ref_count; i++) + for (i = 0; i < unsafe_by_ref_count; i++) { - int idx = bb->index * by_ref_count + i; + int idx = bb->index * unsafe_by_ref_count + i; bool first = true; HOST_WIDE_INT inh = 0; FOR_EACH_EDGE (e, ei, bb->succs) { - int succ_idx = e->dest->index * by_ref_count + i; + int succ_idx = e->dest->index * unsafe_by_ref_count + i; if (e->dest == EXIT_BLOCK_PTR_FOR_FN (fun)) continue; @@ -2184,13 +2199,24 @@ propagate_dereference_distances (struct function *fun) "Dereference table after propagation:\n"); } -/* Perform basic checks on ACCESS to PARM described by DESC and all its - children, return true if the parameter cannot be split, otherwise return - true and update *TOTAL_SIZE and *ONLY_CALLS. ENTRY_BB_INDEX must be the - index of the entry BB in the function of PARM. */ +/* Return true if the ACCESS loads happen frequently enough in FUN to risk + moving them to the caller and only pass the result. */ static bool -check_gensum_access (tree parm, gensum_param_desc *desc, +dereference_probable_p (struct function *fun, gensum_param_access *access) +{ + int threshold = opt_for_fn (fun->decl, param_ipa_sra_deref_prob_threshold); + return access->load_count + >= ENTRY_BLOCK_PTR_FOR_FN (fun)->count.apply_scale (threshold, 100); +} + +/* Perform basic checks on ACCESS to PARM (of FUN) described by DESC and all + its children, return true if the parameter cannot be split, otherwise return + false and update *NONARG_ACC_SIZE and *ONLY_CALLS. ENTRY_BB_INDEX must be + the index of the entry BB in the function of PARM. */ + +static bool +check_gensum_access (struct function *fun, tree parm, gensum_param_desc *desc, gensum_param_access *access, HOST_WIDE_INT *nonarg_acc_size, bool *only_calls, int entry_bb_index) @@ -2218,19 +2244,31 @@ check_gensum_access (tree parm, gensum_param_desc *desc, if (desc->by_ref) { - int idx = (entry_bb_index * by_ref_count + desc->deref_index); - if ((access->offset + access->size) > bb_dereferences[idx]) + if (desc->safe_ref) { - disqualify_split_candidate (desc, "Would create a possibly " - "illegal dereference in a caller."); - return true; + if (!dereference_probable_p (fun, access)) + { + disqualify_split_candidate (desc, "Dereferences in callers " + "would happen much more frequently."); + return true; + } + } + else + { + int idx = (entry_bb_index * unsafe_by_ref_count + desc->deref_index); + if ((access->offset + access->size) > bb_dereferences[idx]) + { + disqualify_split_candidate (desc, "Would create a possibly " + "illegal dereference in a caller."); + return true; + } } } for (gensum_param_access *ch = access->first_child; ch; ch = ch->next_sibling) - if (check_gensum_access (parm, desc, ch, nonarg_acc_size, only_calls, + if (check_gensum_access (fun, parm, desc, ch, nonarg_acc_size, only_calls, entry_bb_index)) return true; @@ -2288,6 +2326,7 @@ process_scan_results (cgraph_node *node, struct function *fun, if (!dereferences_propagated && desc->by_ref + && !desc->safe_ref && desc->accesses) { propagate_dereference_distances (fun); @@ -2302,8 +2341,8 @@ process_scan_results (cgraph_node *node, struct function *fun, for (gensum_param_access *acc = desc->accesses; acc; acc = acc->next_sibling) - if (check_gensum_access (parm, desc, acc, &nonarg_acc_size, &only_calls, - entry_bb_index)) + if (check_gensum_access (fun, parm, desc, acc, &nonarg_acc_size, + &only_calls, entry_bb_index)) { check_failed = true; break; @@ -2394,6 +2433,12 @@ process_scan_results (cgraph_node *node, struct function *fun, if (!uses_memory_as_obtained) continue; + if (desc->safe_ref) + { + csum->m_arg_flow[argidx].safe_to_import_accesses = true; + continue; + } + /* Post-dominator check placed last, hoping that it usually won't be needed. */ if (!pdoms_calculated) @@ -4124,7 +4169,7 @@ ipa_sra_summarize_function (cgraph_node *node) cfun_pushed = true; final_bbs = BITMAP_ALLOC (NULL); bb_dereferences = XCNEWVEC (HOST_WIDE_INT, - by_ref_count + unsafe_by_ref_count * last_basic_block_for_fn (fun)); aa_walking_limit = opt_for_fn (node->decl, param_ipa_max_aa_steps); scan_function (node, fun); diff --git a/gcc/params.opt b/gcc/params.opt index 397ec0bd128..a574f17bddb 100644 --- a/gcc/params.opt +++ b/gcc/params.opt @@ -282,6 +282,10 @@ Maximum number of different predicates used to track properties of loops in IPA Common Joined UInteger Var(param_ipa_max_switch_predicate_bounds) Init(5) Param Optimization Maximal number of boundary endpoints of case ranges of switch statement used during IPA function summary generation. +-param=ipa-sra-deref-prob-threshold= +Common Joined UInteger Var(param_ipa_sra_deref_prob_threshold) Init(50) IntegerRange(0, 100) Param Optimization +Minimum probability (in percent) of dereferencing of a function pointer parameter for it to be considered for replacement with simple values. + -param=ipa-sra-max-replacements= Common Joined UInteger Var(param_ipa_sra_max_replacements) Optimization Init(8) IntegerRange(0, 16) Param Maximum pieces that IPA-SRA tracks per formal parameter, as a consequence, also the maximum number of replacements of a formal parameter. diff --git a/gcc/testsuite/g++.dg/ipa/ipa-sra-5.C b/gcc/testsuite/g++.dg/ipa/ipa-sra-5.C new file mode 100644 index 00000000000..26a221e1734 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/ipa-sra-5.C @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-sra" } */ + +volatile int vi; + +static void __attribute__((noinline)) +foo (int c, int &r) +{ + int i; + if (c) + i = r; + else + i = 0; + vi = i; +} + +void +bar (int c, int j) +{ + foo (c, j); +} + +/* { dg-final { scan-ipa-dump "Will split parameter" "sra" } } */
diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc index 0f137e810fe..e8a4cd47429 100644 --- a/gcc/ipa-sra.cc +++ b/gcc/ipa-sra.cc @@ -151,6 +151,8 @@ struct gensum_param_access arguments. */ tree alias_ptr_type; + /* Cumulative count of all loads. */ + profile_count load_count; /* Have there been writes to or reads from this exact location except for as arguments to a function call that can be tracked. */ bool nonarg; @@ -207,8 +209,13 @@ struct gensum_param_desc by reference that is a candidate for being converted to a set of parameters passing those data by value. */ bool split_candidate; - /* Is this a parameter passing stuff by reference? */ + /* Is this a parameter passing stuff by reference (either a pointer or a + source language reference type)? */ bool by_ref; + /* If this parameter passes stuff by reference, can it be safely dereferenced + without performing further checks (for example because it is a + REFERENCE_TYPE)? */ + bool safe_ref; /* The number of this parameter as they are ordered in function decl. */ int param_number; @@ -561,7 +568,7 @@ int aa_walking_limit; accessed in that BB. */ HOST_WIDE_INT *bb_dereferences = NULL; /* How many by-reference parameters there are in the current function. */ -int by_ref_count; +int unsafe_by_ref_count; /* Bitmap of BBs that can cause the function to "stop" progressing by returning, throwing externally, looping infinitely or calling a function @@ -643,6 +650,8 @@ dump_gensum_access (FILE *f, gensum_param_access *access, unsigned indent) print_generic_expr (f, access->type); fprintf (f, ", alias_ptr_type: "); print_generic_expr (f, access->alias_ptr_type); + fprintf (f, ", load_count: "); + access->load_count.dump (f); fprintf (f, ", nonarg: %u, reverse: %u\n", access->nonarg, access->reverse); for (gensum_param_access *ch = access->first_child; ch; @@ -692,7 +701,8 @@ dump_gensum_param_descriptor (FILE *f, gensum_param_desc *desc) return; } if (desc->by_ref) - fprintf (f, " by_ref with %u pass throughs\n", desc->ptr_pt_count); + fprintf (f, " %s by_ref with %u pass throughs\n", + desc->safe_ref ? "safe" : "unsafe", desc->ptr_pt_count); for (gensum_param_access *acc = desc->accesses; acc; acc = acc->next_sibling) dump_gensum_access (f, acc, 2); @@ -1140,6 +1150,8 @@ create_parameter_descriptors (cgraph_node *node, if (POINTER_TYPE_P (type)) { + desc->by_ref = true; + desc->safe_ref = (TREE_CODE (type) == REFERENCE_TYPE); type = TREE_TYPE (type); if (TREE_CODE (type) == FUNCTION_TYPE @@ -1187,7 +1199,6 @@ create_parameter_descriptors (cgraph_node *node, "nonarg uses\n"); continue; } - desc->by_ref = true; } else if (!AGGREGATE_TYPE_P (type)) { @@ -1231,8 +1242,8 @@ create_parameter_descriptors (cgraph_node *node, ret = true; desc->split_candidate = true; - if (desc->by_ref) - desc->deref_index = by_ref_count++; + if (desc->by_ref && !desc->safe_ref) + desc->deref_index = unsafe_by_ref_count++; } return ret; } @@ -1301,6 +1312,7 @@ allocate_access (gensum_param_desc *desc, memset (access, 0, sizeof (*access)); access->offset = offset; access->size = size; + access->load_count = profile_count::zero (); return access; } @@ -1555,15 +1567,16 @@ asm_visit_addr (gimple *, tree op, tree, void *) static void mark_param_dereference (gensum_param_desc *desc, HOST_WIDE_INT dist, - basic_block bb) + basic_block bb) { gcc_assert (desc->by_ref); gcc_checking_assert (desc->split_candidate); - if (bitmap_bit_p (final_bbs, bb->index)) + if (desc->safe_ref + || bitmap_bit_p (final_bbs, bb->index)) return; - int idx = bb->index * by_ref_count + desc->deref_index; + int idx = bb->index * unsafe_by_ref_count + desc->deref_index; if (bb_dereferences[idx] < dist) bb_dereferences[idx] = dist; } @@ -1811,6 +1824,8 @@ scan_expr_access (tree expr, gimple *stmt, isra_scan_context ctx, other. */ access->nonarg = true; } + else if (ctx == ISRA_CTX_LOAD && bb->count.initialized_p ()) + access->load_count += bb->count; if (!access->type) { @@ -2092,9 +2107,9 @@ dump_dereferences_table (FILE *f, struct function *fun, const char *str) if (bb != EXIT_BLOCK_PTR_FOR_FN (fun)) { int i; - for (i = 0; i < by_ref_count; i++) + for (i = 0; i < unsafe_by_ref_count; i++) { - int idx = bb->index * by_ref_count + i; + int idx = bb->index * unsafe_by_ref_count + i; fprintf (f, " %4" HOST_WIDE_INT_PRINT "d", bb_dereferences[idx]); } } @@ -2139,15 +2154,15 @@ propagate_dereference_distances (struct function *fun) if (bitmap_bit_p (final_bbs, bb->index)) continue; - for (i = 0; i < by_ref_count; i++) + for (i = 0; i < unsafe_by_ref_count; i++) { - int idx = bb->index * by_ref_count + i; + int idx = bb->index * unsafe_by_ref_count + i; bool first = true; HOST_WIDE_INT inh = 0; FOR_EACH_EDGE (e, ei, bb->succs) { - int succ_idx = e->dest->index * by_ref_count + i; + int succ_idx = e->dest->index * unsafe_by_ref_count + i; if (e->dest == EXIT_BLOCK_PTR_FOR_FN (fun)) continue; @@ -2184,13 +2199,23 @@ propagate_dereference_distances (struct function *fun) "Dereference table after propagation:\n"); } -/* Perform basic checks on ACCESS to PARM described by DESC and all its - children, return true if the parameter cannot be split, otherwise return - true and update *TOTAL_SIZE and *ONLY_CALLS. ENTRY_BB_INDEX must be the - index of the entry BB in the function of PARM. */ +/* Return true if the ACCESS loads happen frequently enough in FUN to risk + moving them to the caller and only pass the result. */ static bool -check_gensum_access (tree parm, gensum_param_desc *desc, +dereference_probable_p (struct function *fun, gensum_param_access *access) +{ + return access->load_count + >= ENTRY_BLOCK_PTR_FOR_FN (fun)->count.apply_scale (1, 2); +} + +/* Perform basic checks on ACCESS to PARM (of FUN) described by DESC and all + its children, return true if the parameter cannot be split, otherwise return + false and update *NONARG_ACC_SIZE and *ONLY_CALLS. ENTRY_BB_INDEX must be + the index of the entry BB in the function of PARM. */ + +static bool +check_gensum_access (struct function *fun, tree parm, gensum_param_desc *desc, gensum_param_access *access, HOST_WIDE_INT *nonarg_acc_size, bool *only_calls, int entry_bb_index) @@ -2218,19 +2243,31 @@ check_gensum_access (tree parm, gensum_param_desc *desc, if (desc->by_ref) { - int idx = (entry_bb_index * by_ref_count + desc->deref_index); - if ((access->offset + access->size) > bb_dereferences[idx]) + if (desc->safe_ref) { - disqualify_split_candidate (desc, "Would create a possibly " - "illegal dereference in a caller."); - return true; + if (!dereference_probable_p (fun, access)) + { + disqualify_split_candidate (desc, "Dereferences in callers " + "would happen much more frequently."); + return true; + } + } + else + { + int idx = (entry_bb_index * unsafe_by_ref_count + desc->deref_index); + if ((access->offset + access->size) > bb_dereferences[idx]) + { + disqualify_split_candidate (desc, "Would create a possibly " + "illegal dereference in a caller."); + return true; + } } } for (gensum_param_access *ch = access->first_child; ch; ch = ch->next_sibling) - if (check_gensum_access (parm, desc, ch, nonarg_acc_size, only_calls, + if (check_gensum_access (fun, parm, desc, ch, nonarg_acc_size, only_calls, entry_bb_index)) return true; @@ -2288,6 +2325,7 @@ process_scan_results (cgraph_node *node, struct function *fun, if (!dereferences_propagated && desc->by_ref + && !desc->safe_ref && desc->accesses) { propagate_dereference_distances (fun); @@ -2302,8 +2340,8 @@ process_scan_results (cgraph_node *node, struct function *fun, for (gensum_param_access *acc = desc->accesses; acc; acc = acc->next_sibling) - if (check_gensum_access (parm, desc, acc, &nonarg_acc_size, &only_calls, - entry_bb_index)) + if (check_gensum_access (fun, parm, desc, acc, &nonarg_acc_size, + &only_calls, entry_bb_index)) { check_failed = true; break; @@ -2394,6 +2432,12 @@ process_scan_results (cgraph_node *node, struct function *fun, if (!uses_memory_as_obtained) continue; + if (desc->safe_ref) + { + csum->m_arg_flow[argidx].safe_to_import_accesses = true; + continue; + } + /* Post-dominator check placed last, hoping that it usually won't be needed. */ if (!pdoms_calculated) @@ -4124,7 +4168,7 @@ ipa_sra_summarize_function (cgraph_node *node) cfun_pushed = true; final_bbs = BITMAP_ALLOC (NULL); bb_dereferences = XCNEWVEC (HOST_WIDE_INT, - by_ref_count + unsafe_by_ref_count * last_basic_block_for_fn (fun)); aa_walking_limit = opt_for_fn (node->decl, param_ipa_max_aa_steps); scan_function (node, fun); diff --git a/gcc/testsuite/g++.dg/ipa/ipa-sra-5.C b/gcc/testsuite/g++.dg/ipa/ipa-sra-5.C new file mode 100644 index 00000000000..26a221e1734 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/ipa-sra-5.C @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-sra" } */ + +volatile int vi; + +static void __attribute__((noinline)) +foo (int c, int &r) +{ + int i; + if (c) + i = r; + else + i = 0; + vi = i; +} + +void +bar (int c, int j) +{ + foo (c, j); +} + +/* { dg-final { scan-ipa-dump "Will split parameter" "sra" } } */
Hi, I'm re-posting patches which I have posted at the end of stage 1 but which have not passed review yet. 8<-------------------------------------------------------------------- C++ and especially Fortran pass data by references which are not pointers potentially pointing anywhere and so can be assumed to be safely dereferencable. This patch teaches IPA-SRA to treat them as such and avoid the dance we do to prove that we can move loads from them to the caller. When we do not know that a dereference will happen all the time, we need a heuristics so that we do not force memory accesses that normally happen only rarely. The patch simply uses the (possibly guessed) profile and checks whether the (expected) number of loads is at least half of function invocations invocations. Bootstrapped and tested individually when I originally posted it and now bootstrapped and LTO-bootstrapped and tested as part of the whole series. OK for master? gcc/ChangeLog: 2022-11-11 Martin Jambor <mjambor@suse.cz> PR ipa/103585 * ipa-sra.c (struct gensum_param_access): New field load_count. (struct gensum_param_desc): New field safe_ref, adjusted comments. (by_ref_count): Renamed to unsafe_by_ref_count, adjusted all uses. (dump_gensum_access): Dump the new field. (dump_gensum_param_descriptor): Likewise. (create_parameter_descriptors): Set safe_ref field, move setting by_ref forward. Only increment unsafe_by_ref_count for unsafe by_ref parameters. (allocate_access): Initialize new field. (mark_param_dereference): Adjust indentation. Only add data to bb_dereferences for unsafe by_ref parameters. (scan_expr_access): For loads, accumulate BB counts. (dereference_probable_p): New function. (check_gensum_access): Fix leading comment, add parameter FUN. Check cumulative counts of loads for safe by_ref accesses instead of dereferences. (process_scan_results): Do not propagate dereference distances for safe by_ref parameters. Pass fun to check_gensum_access. Safe by_ref params do not need the postdominance check. gcc/testsuite/ChangeLog: 2022-11-11 Martin Jambor <mjambor@suse.cz> * g++.dg/ipa/ipa-sra-5.C: New test --- gcc/ipa-sra.cc | 100 +++++++++++++++++++-------- gcc/testsuite/g++.dg/ipa/ipa-sra-5.C | 23 ++++++ 2 files changed, 95 insertions(+), 28 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/ipa-sra-5.C