* [PATCH] Fix UBSAN errors in ipa-cp. @ 2020-09-23 12:19 Martin Liška 2020-09-23 12:32 ` Jan Hubicka 0 siblings, 1 reply; 8+ messages in thread From: Martin Liška @ 2020-09-23 12:19 UTC (permalink / raw) To: gcc-patches; +Cc: Martin Jambor, Jan Hubicka I see the following UBSAN errors: ./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/ipa/pr96806.C -std=c++11 -O -fipa-cp -fipa-cp-clone --param=ipa-cp-max-recursive-depth=94 --param=logical-op-non-short-circuit=0 /home/marxin/Programming/gcc2/gcc/ipa-cp.c:3866:20: runtime error: signed integer overflow: 64 + 2147483584 cannot be represented in type 'int' /home/marxin/Programming/gcc2/gcc/ipa-cp.c:3843:16: runtime error: signed integer overflow: -2147483648 + -2147483648 cannot be represented in type 'int' /home/marxin/Programming/gcc2/gcc/ipa-cp.c:3864:20: runtime error: signed integer overflow: 1 + 2147483647 cannot be represented in type 'int' Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: * ipa-cp.c (safe_add): Handle also very small negative values. (value_topo_info::propagate_effects): Use properly safe_add. --- gcc/ipa-cp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index b3e7d41ea10..e39ee28726d 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -3832,13 +3832,15 @@ propagate_constants_topo (class ipa_topo_info *topo) /* Return the sum of A and B if none of them is bigger than INT_MAX/2, return - the bigger one if otherwise. */ + the bigger one if otherwise. Similarly for negative numbers. */ static int safe_add (int a, int b) { if (a > INT_MAX/2 || b > INT_MAX/2) return a > b ? a : b; + else if (a < -INT_MAX/2 || b < -INT_MAX/2) + return a > b ? b : a; else return a + b; } @@ -3861,9 +3863,10 @@ value_topo_info<valtype>::propagate_effects () for (val = base; val; val = val->scc_next) { - time = safe_add (time, - val->local_time_benefit + val->prop_time_benefit); - size = safe_add (size, val->local_size_cost + val->prop_size_cost); + time = safe_add (time, val->local_time_benefit); + time = safe_add (time, val->prop_time_benefit); + size = safe_add (size, val->local_size_cost); + size = safe_add (size, val->prop_size_cost); } for (val = base; val; val = val->scc_next) -- 2.28.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 12:19 [PATCH] Fix UBSAN errors in ipa-cp Martin Liška @ 2020-09-23 12:32 ` Jan Hubicka 2020-09-23 12:39 ` Martin Liška 0 siblings, 1 reply; 8+ messages in thread From: Jan Hubicka @ 2020-09-23 12:32 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches > I see the following UBSAN errors: > > ./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/ipa/pr96806.C -std=c++11 -O -fipa-cp -fipa-cp-clone --param=ipa-cp-max-recursive-depth=94 --param=logical-op-non-short-circuit=0 > /home/marxin/Programming/gcc2/gcc/ipa-cp.c:3866:20: runtime error: signed integer overflow: 64 + 2147483584 cannot be represented in type 'int' > /home/marxin/Programming/gcc2/gcc/ipa-cp.c:3843:16: runtime error: signed integer overflow: -2147483648 + -2147483648 cannot be represented in type 'int' > /home/marxin/Programming/gcc2/gcc/ipa-cp.c:3864:20: runtime error: signed integer overflow: 1 + 2147483647 cannot be represented in type 'int' > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > * ipa-cp.c (safe_add): Handle also very small negative values. > (value_topo_info::propagate_effects): Use properly safe_add. Perhaps it is time to turn the profile count scaled valued to sreals like we do in inline heuristics and other places? Honza > --- > gcc/ipa-cp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c > index b3e7d41ea10..e39ee28726d 100644 > --- a/gcc/ipa-cp.c > +++ b/gcc/ipa-cp.c > @@ -3832,13 +3832,15 @@ propagate_constants_topo (class ipa_topo_info *topo) > /* Return the sum of A and B if none of them is bigger than INT_MAX/2, return > - the bigger one if otherwise. */ > + the bigger one if otherwise. Similarly for negative numbers. */ > static int > safe_add (int a, int b) > { > if (a > INT_MAX/2 || b > INT_MAX/2) > return a > b ? a : b; > + else if (a < -INT_MAX/2 || b < -INT_MAX/2) > + return a > b ? b : a; > else > return a + b; > } > @@ -3861,9 +3863,10 @@ value_topo_info<valtype>::propagate_effects () > for (val = base; val; val = val->scc_next) > { > - time = safe_add (time, > - val->local_time_benefit + val->prop_time_benefit); > - size = safe_add (size, val->local_size_cost + val->prop_size_cost); > + time = safe_add (time, val->local_time_benefit); > + time = safe_add (time, val->prop_time_benefit); > + size = safe_add (size, val->local_size_cost); > + size = safe_add (size, val->prop_size_cost); > } > for (val = base; val; val = val->scc_next) > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 12:32 ` Jan Hubicka @ 2020-09-23 12:39 ` Martin Liška 2020-09-23 13:50 ` Martin Liška 0 siblings, 1 reply; 8+ messages in thread From: Martin Liška @ 2020-09-23 12:39 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On 9/23/20 2:32 PM, Jan Hubicka wrote: > Perhaps it is time to turn the profile count scaled valued to sreals > like we do in inline heuristics and other places? Yep, I'll try to come up with a patch. Martin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 12:39 ` Martin Liška @ 2020-09-23 13:50 ` Martin Liška 2020-09-23 13:57 ` Jan Hubicka 2020-09-23 15:02 ` Martin Jambor 0 siblings, 2 replies; 8+ messages in thread From: Martin Liška @ 2020-09-23 13:50 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Martin Jambor [-- Attachment #1: Type: text/plain, Size: 141 bytes --] There's patch that does that. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin [-- Attachment #2: 0001-Port-IPA-CP-time-and-size-to-sreal.patch --] [-- Type: text/x-patch, Size: 12350 bytes --] From ff5f78110684ed9aedde15d19e856b3acf649971 Mon Sep 17 00:00:00 2001 From: Martin Liska <mliska@suse.cz> Date: Wed, 23 Sep 2020 15:10:43 +0200 Subject: [PATCH] Port IPA CP time and size to sreal gcc/ChangeLog: * ipa-cp.c (ipcp_lattice::print): Print sreal values with to_double. (incorporate_penalties): Change type to sreal. (good_cloning_opportunity_p): Change args to sreal. (get_max_overall_size): Work in sreal type. (estimate_local_effects): Likewise. (safe_add): Remove. (value_topo_info::propagate_effects): Work in sreal type. (ipcp_propagate_stage): Print sreal numbers. (decide_about_value): Work in sreal type. --- gcc/ipa-cp.c | 128 ++++++++++++++++++++++++--------------------------- 1 file changed, 59 insertions(+), 69 deletions(-) diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index b3e7d41ea10..9a79b5862f8 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -157,10 +157,10 @@ class ipcp_value_base public: /* Time benefit and size cost that specializing the function for this value would bring about in this function alone. */ - int local_time_benefit, local_size_cost; + sreal local_time_benefit, local_size_cost; /* Time benefit and size cost that specializing the function for this value can bring about in it's callees (transitively). */ - int prop_time_benefit, prop_size_cost; + sreal prop_time_benefit, prop_size_cost; ipcp_value_base () : local_time_benefit (0), local_size_cost (0), @@ -376,7 +376,7 @@ static profile_count max_count; /* Original overall size of the program. */ -static long overall_size, orig_overall_size; +static sreal overall_size, orig_overall_size; /* Node name to unique clone suffix number map. */ static hash_map<const char *, unsigned> *clone_num_suffixes; @@ -498,10 +498,12 @@ ipcp_lattice<valtype>::print (FILE * f, bool dump_sources, bool dump_benefits) } if (dump_benefits) - fprintf (f, " [loc_time: %i, loc_size: %i, " - "prop_time: %i, prop_size: %i]\n", - val->local_time_benefit, val->local_size_cost, - val->prop_time_benefit, val->prop_size_cost); + fprintf (f, " [loc_time: %f, loc_size: %f, " + "prop_time: %f, prop_size: %f]\n", + val->local_time_benefit.to_double (), + val->local_size_cost.to_double (), + val->prop_time_benefit.to_double (), + val->prop_size_cost.to_double ()); } if (!dump_benefits) fprintf (f, "\n"); @@ -3201,9 +3203,9 @@ hint_time_bonus (cgraph_node *node, ipa_hints hints) /* If there is a reason to penalize the function described by INFO in the cloning goodness evaluation, do so. */ -static inline int64_t +static inline sreal incorporate_penalties (cgraph_node *node, ipa_node_params *info, - int64_t evaluation) + sreal evaluation) { if (info->node_within_scc && !info->node_is_self_scc) evaluation = (evaluation @@ -3224,8 +3226,9 @@ incorporate_penalties (cgraph_node *node, ipa_node_params *info, potential new clone in FREQUENCIES. */ static bool -good_cloning_opportunity_p (struct cgraph_node *node, int time_benefit, - int freq_sum, profile_count count_sum, int size_cost) +good_cloning_opportunity_p (struct cgraph_node *node, sreal time_benefit, + sreal freq_sum, profile_count count_sum, + sreal size_cost) { if (time_benefit == 0 || !opt_for_fn (node->decl, flag_ipa_cp_clone) @@ -3241,40 +3244,39 @@ good_cloning_opportunity_p (struct cgraph_node *node, int time_benefit, int factor = RDIV (count_sum.probability_in (max_count).to_reg_br_prob_base () * 1000, REG_BR_PROB_BASE); - int64_t evaluation = (((int64_t) time_benefit * factor) - / size_cost); + sreal evaluation = (time_benefit * factor) / size_cost; evaluation = incorporate_penalties (node, info, evaluation); if (dump_file && (dump_flags & TDF_DETAILS)) { - fprintf (dump_file, " good_cloning_opportunity_p (time: %i, " - "size: %i, count_sum: ", time_benefit, size_cost); + fprintf (dump_file, " good_cloning_opportunity_p (time: %f, " + "size: %f, count_sum: ", time_benefit.to_double (), + size_cost.to_double ()); count_sum.dump (dump_file); - fprintf (dump_file, "%s%s) -> evaluation: " "%" PRId64 - ", threshold: %i\n", + fprintf (dump_file, "%s%s) -> evaluation: %f, threshold: %i\n", info->node_within_scc ? (info->node_is_self_scc ? ", self_scc" : ", scc") : "", info->node_calling_single_call ? ", single_call" : "", - evaluation, eval_threshold); + evaluation.to_double (), eval_threshold); } return evaluation >= eval_threshold; } else { - int64_t evaluation = (((int64_t) time_benefit * freq_sum) - / size_cost); + sreal evaluation = (time_benefit * freq_sum) / size_cost; evaluation = incorporate_penalties (node, info, evaluation); if (dump_file && (dump_flags & TDF_DETAILS)) - fprintf (dump_file, " good_cloning_opportunity_p (time: %i, " - "size: %i, freq_sum: %i%s%s) -> evaluation: " - "%" PRId64 ", threshold: %i\n", - time_benefit, size_cost, freq_sum, + fprintf (dump_file, " good_cloning_opportunity_p (time: %f, " + "size: %f, freq_sum: %f%s%s) -> evaluation: %f" + ", threshold: %i\n", + time_benefit.to_double (), size_cost.to_double (), + freq_sum.to_double (), info->node_within_scc ? (info->node_is_self_scc ? ", self_scc" : ", scc") : "", info->node_calling_single_call ? ", single_call" : "", - evaluation, eval_threshold); + evaluation.to_double (), eval_threshold); return evaluation >= eval_threshold; } @@ -3434,10 +3436,10 @@ perform_estimation_of_a_value (cgraph_node *node, vec<tree> known_csts, limits but it is possible and if it happens, we do not want to select one limit at random. */ -static long +static sreal get_max_overall_size (cgraph_node *node) { - long max_new_size = orig_overall_size; + sreal max_new_size = orig_overall_size; long large_unit = opt_for_fn (node->decl, param_large_unit_insns); if (max_new_size < large_unit) max_new_size = large_unit; @@ -3476,23 +3478,25 @@ estimate_local_effects (struct cgraph_node *node) { struct caller_statistics stats; ipa_hints hints; - sreal time, base_time; - int size; + sreal time, base_time, size; + int int_size; init_caller_stats (&stats); node->call_for_symbol_thunks_and_aliases (gather_caller_stats, &stats, false); estimate_ipcp_clone_size_and_time (node, known_csts, known_contexts, - known_aggs, &size, &time, + known_aggs, &int_size, &time, &base_time, &hints); + size = int_size; time -= devirt_bonus; time -= hint_time_bonus (node, hints); time -= removable_params_cost; size -= stats.n_calls * removable_params_cost; if (dump_file) - fprintf (dump_file, " - context independent values, size: %i, " - "time_benefit: %f\n", size, (base_time - time).to_double ()); + fprintf (dump_file, " - context independent values, size: %f, " + "time_benefit: %f\n", size.to_double (), + (base_time - time).to_double ()); if (size <= 0 || node->local) { @@ -3519,8 +3523,8 @@ estimate_local_effects (struct cgraph_node *node) } else if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " Not cloning for all contexts because " - "maximum unit size would be reached with %li.\n", - size + overall_size); + "maximum unit size would be reached with %f.\n", + (size + overall_size).to_double ()); } else if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " Not cloning for all contexts because " @@ -3555,8 +3559,9 @@ estimate_local_effects (struct cgraph_node *node) print_ipcp_constant_value (dump_file, val->value); fprintf (dump_file, " for "); ipa_dump_param (dump_file, info, i); - fprintf (dump_file, ": time_benefit: %i, size: %i\n", - val->local_time_benefit, val->local_size_cost); + fprintf (dump_file, ": time_benefit: %f, size: %f\n", + val->local_time_benefit.to_double (), + val->local_size_cost.to_double ()); } } known_csts[i] = NULL_TREE; @@ -3590,8 +3595,9 @@ estimate_local_effects (struct cgraph_node *node) print_ipcp_constant_value (dump_file, val->value); fprintf (dump_file, " for "); ipa_dump_param (dump_file, info, i); - fprintf (dump_file, ": time_benefit: %i, size: %i\n", - val->local_time_benefit, val->local_size_cost); + fprintf (dump_file, ": time_benefit: %f, size: %f\n", + val->local_time_benefit.to_double (), + val->local_size_cost.to_double ()); } } known_contexts[i] = ipa_polymorphic_call_context (); @@ -3635,10 +3641,11 @@ estimate_local_effects (struct cgraph_node *node) fprintf (dump_file, " for "); ipa_dump_param (dump_file, info, i); fprintf (dump_file, "[%soffset: " HOST_WIDE_INT_PRINT_DEC - "]: time_benefit: %i, size: %i\n", + "]: time_benefit: %f, size: %f\n", plats->aggs_by_ref ? "ref " : "", aglat->offset, - val->local_time_benefit, val->local_size_cost); + val->local_time_benefit.to_double (), + val->local_size_cost.to_double ()); } agg->items.pop (); @@ -3830,20 +3837,6 @@ propagate_constants_topo (class ipa_topo_info *topo) } } - -/* Return the sum of A and B if none of them is bigger than INT_MAX/2, return - the bigger one if otherwise. */ - -static int -safe_add (int a, int b) -{ - if (a > INT_MAX/2 || b > INT_MAX/2) - return a > b ? a : b; - else - return a + b; -} - - /* Propagate the estimated effects of individual values along the topological from the dependent values to those they depend on. */ @@ -3857,13 +3850,12 @@ value_topo_info<valtype>::propagate_effects () { ipcp_value_source<valtype> *src; ipcp_value<valtype> *val; - int time = 0, size = 0; + sreal time = 0, size = 0; for (val = base; val; val = val->scc_next) { - time = safe_add (time, - val->local_time_benefit + val->prop_time_benefit); - size = safe_add (size, val->local_size_cost + val->prop_size_cost); + time += val->local_time_benefit + val->prop_time_benefit; + size += val->local_size_cost + val->prop_size_cost; } for (val = base; val; val = val->scc_next) @@ -3871,10 +3863,8 @@ value_topo_info<valtype>::propagate_effects () if (src->val && src->cs->maybe_hot_p ()) { - src->val->prop_time_benefit = safe_add (time, - src->val->prop_time_benefit); - src->val->prop_size_cost = safe_add (size, - src->val->prop_size_cost); + src->val->prop_time_benefit = time + src->val->prop_time_benefit; + src->val->prop_size_cost = size + src->val->prop_size_cost; } } } @@ -3916,7 +3906,7 @@ ipcp_propagate_stage (class ipa_topo_info *topo) orig_overall_size = overall_size; if (dump_file) - fprintf (dump_file, "\noverall_size: %li\n", overall_size); + fprintf (dump_file, "\noverall_size: %f\n", overall_size.to_double ()); propagate_constants_topo (topo); if (flag_checking) @@ -5458,8 +5448,8 @@ decide_about_value (struct cgraph_node *node, int index, HOST_WIDE_INT offset, { if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " Ignoring candidate value because " - "maximum unit size would be reached with %li.\n", - val->local_size_cost + overall_size); + "maximum unit size would be reached with %f.\n", + (val->local_size_cost + overall_size).to_double ()); return false; } else if (!get_info_about_necessary_edges (val, node, &freq_sum, &count_sum, @@ -5481,11 +5471,11 @@ decide_about_value (struct cgraph_node *node, int index, HOST_WIDE_INT offset, freq_sum, count_sum, val->local_size_cost) && !good_cloning_opportunity_p (node, - safe_add (val->local_time_benefit, - val->prop_time_benefit), + val->local_time_benefit + + val->prop_time_benefit, freq_sum, count_sum, - safe_add (val->local_size_cost, - val->prop_size_cost))) + val->local_size_cost + + val->prop_size_cost)) return false; if (dump_file) -- 2.28.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 13:50 ` Martin Liška @ 2020-09-23 13:57 ` Jan Hubicka 2020-09-23 14:32 ` Martin Jambor 2020-09-23 15:02 ` Martin Jambor 1 sibling, 1 reply; 8+ messages in thread From: Jan Hubicka @ 2020-09-23 13:57 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches, Martin Jambor > There's patch that does that. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > From ff5f78110684ed9aedde15d19e856b3acf649971 Mon Sep 17 00:00:00 2001 > From: Martin Liska <mliska@suse.cz> > Date: Wed, 23 Sep 2020 15:10:43 +0200 > Subject: [PATCH] Port IPA CP time and size to sreal > > gcc/ChangeLog: > > * ipa-cp.c (ipcp_lattice::print): Print sreal values > with to_double. > (incorporate_penalties): Change type to sreal. > (good_cloning_opportunity_p): Change args to sreal. > (get_max_overall_size): Work in sreal type. > (estimate_local_effects): Likewise. > (safe_add): Remove. > (value_topo_info::propagate_effects): Work in sreal type. > (ipcp_propagate_stage): Print sreal numbers. > (decide_about_value): Work in sreal type. > --- > gcc/ipa-cp.c | 128 ++++++++++++++++++++++++--------------------------- > 1 file changed, 59 insertions(+), 69 deletions(-) > > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c > index b3e7d41ea10..9a79b5862f8 100644 > --- a/gcc/ipa-cp.c > +++ b/gcc/ipa-cp.c > @@ -157,10 +157,10 @@ class ipcp_value_base > public: > /* Time benefit and size cost that specializing the function for this value > would bring about in this function alone. */ > - int local_time_benefit, local_size_cost; > + sreal local_time_benefit, local_size_cost; > /* Time benefit and size cost that specializing the function for this value > can bring about in it's callees (transitively). */ > - int prop_time_benefit, prop_size_cost; > + sreal prop_time_benefit, prop_size_cost; Time calculaitons can get out of hand, while size calculations still corresponds to actual program instructions, so int should be enough (I would start using 64bit ints for whole program statistics, like overall size below because eventaully we may hit that). So please keep sizes in integers and times in sreals. > > ipcp_value_base () > : local_time_benefit (0), local_size_cost (0), > @@ -376,7 +376,7 @@ static profile_count max_count; > > /* Original overall size of the program. */ > > -static long overall_size, orig_overall_size; > +static sreal overall_size, orig_overall_size; > > /* Node name to unique clone suffix number map. */ > static hash_map<const char *, unsigned> *clone_num_suffixes; > @@ -498,10 +498,12 @@ ipcp_lattice<valtype>::print (FILE * f, bool dump_sources, bool dump_benefits) > } > > if (dump_benefits) > - fprintf (f, " [loc_time: %i, loc_size: %i, " > - "prop_time: %i, prop_size: %i]\n", > - val->local_time_benefit, val->local_size_cost, > - val->prop_time_benefit, val->prop_size_cost); > + fprintf (f, " [loc_time: %f, loc_size: %f, " > + "prop_time: %f, prop_size: %f]\n", > + val->local_time_benefit.to_double (), > + val->local_size_cost.to_double (), > + val->prop_time_benefit.to_double (), > + val->prop_size_cost.to_double ()); > } > if (!dump_benefits) > fprintf (f, "\n"); > @@ -3201,9 +3203,9 @@ hint_time_bonus (cgraph_node *node, ipa_hints hints) > /* If there is a reason to penalize the function described by INFO in the > cloning goodness evaluation, do so. */ > > -static inline int64_t > +static inline sreal > incorporate_penalties (cgraph_node *node, ipa_node_params *info, > - int64_t evaluation) > + sreal evaluation) > { > if (info->node_within_scc && !info->node_is_self_scc) > evaluation = (evaluation > @@ -3224,8 +3226,9 @@ incorporate_penalties (cgraph_node *node, ipa_node_params *info, > potential new clone in FREQUENCIES. */ > > static bool > -good_cloning_opportunity_p (struct cgraph_node *node, int time_benefit, > - int freq_sum, profile_count count_sum, int size_cost) > +good_cloning_opportunity_p (struct cgraph_node *node, sreal time_benefit, > + sreal freq_sum, profile_count count_sum, > + sreal size_cost) > { > if (time_benefit == 0 > || !opt_for_fn (node->decl, flag_ipa_cp_clone) > @@ -3241,40 +3244,39 @@ good_cloning_opportunity_p (struct cgraph_node *node, int time_benefit, > int factor = RDIV (count_sum.probability_in > (max_count).to_reg_br_prob_base () > * 1000, REG_BR_PROB_BASE); This can be done with to_sreal and you can avoid the scaling by 1000 I think. Note that there are other not really logical capping done earlier that may go as well. Honza > - int64_t evaluation = (((int64_t) time_benefit * factor) > - / size_cost); > + sreal evaluation = (time_benefit * factor) / size_cost; > evaluation = incorporate_penalties (node, info, evaluation); > > if (dump_file && (dump_flags & TDF_DETAILS)) > { > - fprintf (dump_file, " good_cloning_opportunity_p (time: %i, " > - "size: %i, count_sum: ", time_benefit, size_cost); > + fprintf (dump_file, " good_cloning_opportunity_p (time: %f, " > + "size: %f, count_sum: ", time_benefit.to_double (), > + size_cost.to_double ()); > count_sum.dump (dump_file); > - fprintf (dump_file, "%s%s) -> evaluation: " "%" PRId64 > - ", threshold: %i\n", > + fprintf (dump_file, "%s%s) -> evaluation: %f, threshold: %i\n", > info->node_within_scc > ? (info->node_is_self_scc ? ", self_scc" : ", scc") : "", > info->node_calling_single_call ? ", single_call" : "", > - evaluation, eval_threshold); > + evaluation.to_double (), eval_threshold); > } > > return evaluation >= eval_threshold; > } > else > { > - int64_t evaluation = (((int64_t) time_benefit * freq_sum) > - / size_cost); > + sreal evaluation = (time_benefit * freq_sum) / size_cost; > evaluation = incorporate_penalties (node, info, evaluation); > > if (dump_file && (dump_flags & TDF_DETAILS)) > - fprintf (dump_file, " good_cloning_opportunity_p (time: %i, " > - "size: %i, freq_sum: %i%s%s) -> evaluation: " > - "%" PRId64 ", threshold: %i\n", > - time_benefit, size_cost, freq_sum, > + fprintf (dump_file, " good_cloning_opportunity_p (time: %f, " > + "size: %f, freq_sum: %f%s%s) -> evaluation: %f" > + ", threshold: %i\n", > + time_benefit.to_double (), size_cost.to_double (), > + freq_sum.to_double (), > info->node_within_scc > ? (info->node_is_self_scc ? ", self_scc" : ", scc") : "", > info->node_calling_single_call ? ", single_call" : "", > - evaluation, eval_threshold); > + evaluation.to_double (), eval_threshold); > > return evaluation >= eval_threshold; > } > @@ -3434,10 +3436,10 @@ perform_estimation_of_a_value (cgraph_node *node, vec<tree> known_csts, > limits but it is possible and if it happens, we do not want to select one > limit at random. */ > > -static long > +static sreal > get_max_overall_size (cgraph_node *node) > { > - long max_new_size = orig_overall_size; > + sreal max_new_size = orig_overall_size; > long large_unit = opt_for_fn (node->decl, param_large_unit_insns); > if (max_new_size < large_unit) > max_new_size = large_unit; > @@ -3476,23 +3478,25 @@ estimate_local_effects (struct cgraph_node *node) > { > struct caller_statistics stats; > ipa_hints hints; > - sreal time, base_time; > - int size; > + sreal time, base_time, size; > + int int_size; > > init_caller_stats (&stats); > node->call_for_symbol_thunks_and_aliases (gather_caller_stats, &stats, > false); > estimate_ipcp_clone_size_and_time (node, known_csts, known_contexts, > - known_aggs, &size, &time, > + known_aggs, &int_size, &time, > &base_time, &hints); > + size = int_size; > time -= devirt_bonus; > time -= hint_time_bonus (node, hints); > time -= removable_params_cost; > size -= stats.n_calls * removable_params_cost; > > if (dump_file) > - fprintf (dump_file, " - context independent values, size: %i, " > - "time_benefit: %f\n", size, (base_time - time).to_double ()); > + fprintf (dump_file, " - context independent values, size: %f, " > + "time_benefit: %f\n", size.to_double (), > + (base_time - time).to_double ()); > > if (size <= 0 || node->local) > { > @@ -3519,8 +3523,8 @@ estimate_local_effects (struct cgraph_node *node) > } > else if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, " Not cloning for all contexts because " > - "maximum unit size would be reached with %li.\n", > - size + overall_size); > + "maximum unit size would be reached with %f.\n", > + (size + overall_size).to_double ()); > } > else if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, " Not cloning for all contexts because " > @@ -3555,8 +3559,9 @@ estimate_local_effects (struct cgraph_node *node) > print_ipcp_constant_value (dump_file, val->value); > fprintf (dump_file, " for "); > ipa_dump_param (dump_file, info, i); > - fprintf (dump_file, ": time_benefit: %i, size: %i\n", > - val->local_time_benefit, val->local_size_cost); > + fprintf (dump_file, ": time_benefit: %f, size: %f\n", > + val->local_time_benefit.to_double (), > + val->local_size_cost.to_double ()); > } > } > known_csts[i] = NULL_TREE; > @@ -3590,8 +3595,9 @@ estimate_local_effects (struct cgraph_node *node) > print_ipcp_constant_value (dump_file, val->value); > fprintf (dump_file, " for "); > ipa_dump_param (dump_file, info, i); > - fprintf (dump_file, ": time_benefit: %i, size: %i\n", > - val->local_time_benefit, val->local_size_cost); > + fprintf (dump_file, ": time_benefit: %f, size: %f\n", > + val->local_time_benefit.to_double (), > + val->local_size_cost.to_double ()); > } > } > known_contexts[i] = ipa_polymorphic_call_context (); > @@ -3635,10 +3641,11 @@ estimate_local_effects (struct cgraph_node *node) > fprintf (dump_file, " for "); > ipa_dump_param (dump_file, info, i); > fprintf (dump_file, "[%soffset: " HOST_WIDE_INT_PRINT_DEC > - "]: time_benefit: %i, size: %i\n", > + "]: time_benefit: %f, size: %f\n", > plats->aggs_by_ref ? "ref " : "", > aglat->offset, > - val->local_time_benefit, val->local_size_cost); > + val->local_time_benefit.to_double (), > + val->local_size_cost.to_double ()); > } > > agg->items.pop (); > @@ -3830,20 +3837,6 @@ propagate_constants_topo (class ipa_topo_info *topo) > } > } > > - > -/* Return the sum of A and B if none of them is bigger than INT_MAX/2, return > - the bigger one if otherwise. */ > - > -static int > -safe_add (int a, int b) > -{ > - if (a > INT_MAX/2 || b > INT_MAX/2) > - return a > b ? a : b; > - else > - return a + b; > -} > - > - > /* Propagate the estimated effects of individual values along the topological > from the dependent values to those they depend on. */ > > @@ -3857,13 +3850,12 @@ value_topo_info<valtype>::propagate_effects () > { > ipcp_value_source<valtype> *src; > ipcp_value<valtype> *val; > - int time = 0, size = 0; > + sreal time = 0, size = 0; > > for (val = base; val; val = val->scc_next) > { > - time = safe_add (time, > - val->local_time_benefit + val->prop_time_benefit); > - size = safe_add (size, val->local_size_cost + val->prop_size_cost); > + time += val->local_time_benefit + val->prop_time_benefit; > + size += val->local_size_cost + val->prop_size_cost; > } > > for (val = base; val; val = val->scc_next) > @@ -3871,10 +3863,8 @@ value_topo_info<valtype>::propagate_effects () > if (src->val > && src->cs->maybe_hot_p ()) > { > - src->val->prop_time_benefit = safe_add (time, > - src->val->prop_time_benefit); > - src->val->prop_size_cost = safe_add (size, > - src->val->prop_size_cost); > + src->val->prop_time_benefit = time + src->val->prop_time_benefit; > + src->val->prop_size_cost = size + src->val->prop_size_cost; > } > } > } > @@ -3916,7 +3906,7 @@ ipcp_propagate_stage (class ipa_topo_info *topo) > orig_overall_size = overall_size; > > if (dump_file) > - fprintf (dump_file, "\noverall_size: %li\n", overall_size); > + fprintf (dump_file, "\noverall_size: %f\n", overall_size.to_double ()); > > propagate_constants_topo (topo); > if (flag_checking) > @@ -5458,8 +5448,8 @@ decide_about_value (struct cgraph_node *node, int index, HOST_WIDE_INT offset, > { > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, " Ignoring candidate value because " > - "maximum unit size would be reached with %li.\n", > - val->local_size_cost + overall_size); > + "maximum unit size would be reached with %f.\n", > + (val->local_size_cost + overall_size).to_double ()); > return false; > } > else if (!get_info_about_necessary_edges (val, node, &freq_sum, &count_sum, > @@ -5481,11 +5471,11 @@ decide_about_value (struct cgraph_node *node, int index, HOST_WIDE_INT offset, > freq_sum, count_sum, > val->local_size_cost) > && !good_cloning_opportunity_p (node, > - safe_add (val->local_time_benefit, > - val->prop_time_benefit), > + val->local_time_benefit > + + val->prop_time_benefit, > freq_sum, count_sum, > - safe_add (val->local_size_cost, > - val->prop_size_cost))) > + val->local_size_cost > + + val->prop_size_cost)) > return false; > > if (dump_file) > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 13:57 ` Jan Hubicka @ 2020-09-23 14:32 ` Martin Jambor 0 siblings, 0 replies; 8+ messages in thread From: Martin Jambor @ 2020-09-23 14:32 UTC (permalink / raw) To: Jan Hubicka, Martin Liška; +Cc: gcc-patches Hi, On Wed, Sep 23 2020, Jan Hubicka wrote: >> There's patch that does that. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin > >> From ff5f78110684ed9aedde15d19e856b3acf649971 Mon Sep 17 00:00:00 2001 >> From: Martin Liska <mliska@suse.cz> >> Date: Wed, 23 Sep 2020 15:10:43 +0200 >> Subject: [PATCH] Port IPA CP time and size to sreal >> >> gcc/ChangeLog: >> >> * ipa-cp.c (ipcp_lattice::print): Print sreal values >> with to_double. >> (incorporate_penalties): Change type to sreal. >> (good_cloning_opportunity_p): Change args to sreal. >> (get_max_overall_size): Work in sreal type. >> (estimate_local_effects): Likewise. >> (safe_add): Remove. >> (value_topo_info::propagate_effects): Work in sreal type. >> (ipcp_propagate_stage): Print sreal numbers. >> (decide_about_value): Work in sreal type. >> --- >> gcc/ipa-cp.c | 128 ++++++++++++++++++++++++--------------------------- >> 1 file changed, 59 insertions(+), 69 deletions(-) >> >> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c >> index b3e7d41ea10..9a79b5862f8 100644 >> --- a/gcc/ipa-cp.c >> +++ b/gcc/ipa-cp.c >> @@ -157,10 +157,10 @@ class ipcp_value_base >> public: >> /* Time benefit and size cost that specializing the function for this value >> would bring about in this function alone. */ >> - int local_time_benefit, local_size_cost; >> + sreal local_time_benefit, local_size_cost; >> /* Time benefit and size cost that specializing the function for this value >> can bring about in it's callees (transitively). */ >> - int prop_time_benefit, prop_size_cost; >> + sreal prop_time_benefit, prop_size_cost; > > Time calculaitons can get out of hand, while size calculations still > corresponds to actual program instructions, so int should be enough > (I would start using 64bit ints for whole program statistics, like > overall size below because eventaully we may hit that). Please note that in PR 96806 we have seen size overflow int when the value of param ipa-cp-max-recursive-depth is huge. So you either need to keep using safe_add for it or make it a 64 bit int everywhere (and hope that other things break earlier if someone wants to use a value of the param big enough to overflow even 64 bits - or that people testing GCC with strange parameters do not test values this extreme :-). Martin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 13:50 ` Martin Liška 2020-09-23 13:57 ` Jan Hubicka @ 2020-09-23 15:02 ` Martin Jambor 2020-09-24 7:20 ` Martin Liška 1 sibling, 1 reply; 8+ messages in thread From: Martin Jambor @ 2020-09-23 15:02 UTC (permalink / raw) To: Martin Liška, Jan Hubicka; +Cc: gcc-patches Hi, On Wed, Sep 23 2020, Martin Liška wrote: > There's patch that does that. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > From ff5f78110684ed9aedde15d19e856b3acf649971 Mon Sep 17 00:00:00 2001 > From: Martin Liska <mliska@suse.cz> > Date: Wed, 23 Sep 2020 15:10:43 +0200 > Subject: [PATCH] Port IPA CP time and size to sreal > > gcc/ChangeLog: > > * ipa-cp.c (ipcp_lattice::print): Print sreal values > with to_double. > (incorporate_penalties): Change type to sreal. > (good_cloning_opportunity_p): Change args to sreal. > (get_max_overall_size): Work in sreal type. > (estimate_local_effects): Likewise. > (safe_add): Remove. > (value_topo_info::propagate_effects): Work in sreal type. > (ipcp_propagate_stage): Print sreal numbers. > (decide_about_value): Work in sreal type. > --- > gcc/ipa-cp.c | 128 ++++++++++++++++++++++++--------------------------- > 1 file changed, 59 insertions(+), 69 deletions(-) > > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c > index b3e7d41ea10..9a79b5862f8 100644 > @@ -3224,8 +3226,9 @@ incorporate_penalties (cgraph_node *node, ipa_node_params *info, > potential new clone in FREQUENCIES. */ > > static bool > -good_cloning_opportunity_p (struct cgraph_node *node, int time_benefit, > - int freq_sum, profile_count count_sum, int size_cost) > +good_cloning_opportunity_p (struct cgraph_node *node, sreal time_benefit, > + sreal freq_sum, profile_count count_sum, Is the change of type of freq_sum intentional? Even with this patch, all the callers will keep passing their frequency sums as ints and so the implicit conversion seems a bit misleading. I guess complete transition to sreals would include also switch from using cgraph_edge::frequency to sreal_frequency in get_info_about_necessary_edges and users of struct caller_statistics. I planned to work on conversions to sreals right after resolving various exchange2 issues and I can still do this bit afterwards, but the change in the parameter type looks like you wanted to but eventually did not? Thanks, Martin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix UBSAN errors in ipa-cp. 2020-09-23 15:02 ` Martin Jambor @ 2020-09-24 7:20 ` Martin Liška 0 siblings, 0 replies; 8+ messages in thread From: Martin Liška @ 2020-09-24 7:20 UTC (permalink / raw) To: Martin Jambor, Jan Hubicka; +Cc: gcc-patches On 9/23/20 5:02 PM, Martin Jambor wrote: > Hi, > > On Wed, Sep 23 2020, Martin Liška wrote: >> There's patch that does that. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> From ff5f78110684ed9aedde15d19e856b3acf649971 Mon Sep 17 00:00:00 2001 >> From: Martin Liska <mliska@suse.cz> >> Date: Wed, 23 Sep 2020 15:10:43 +0200 >> Subject: [PATCH] Port IPA CP time and size to sreal >> >> gcc/ChangeLog: >> >> * ipa-cp.c (ipcp_lattice::print): Print sreal values >> with to_double. >> (incorporate_penalties): Change type to sreal. >> (good_cloning_opportunity_p): Change args to sreal. >> (get_max_overall_size): Work in sreal type. >> (estimate_local_effects): Likewise. >> (safe_add): Remove. >> (value_topo_info::propagate_effects): Work in sreal type. >> (ipcp_propagate_stage): Print sreal numbers. >> (decide_about_value): Work in sreal type. >> --- >> gcc/ipa-cp.c | 128 ++++++++++++++++++++++++--------------------------- >> 1 file changed, 59 insertions(+), 69 deletions(-) >> >> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c >> index b3e7d41ea10..9a79b5862f8 100644 >> @@ -3224,8 +3226,9 @@ incorporate_penalties (cgraph_node *node, ipa_node_params *info, >> potential new clone in FREQUENCIES. */ >> >> static bool >> -good_cloning_opportunity_p (struct cgraph_node *node, int time_benefit, >> - int freq_sum, profile_count count_sum, int size_cost) >> +good_cloning_opportunity_p (struct cgraph_node *node, sreal time_benefit, >> + sreal freq_sum, profile_count count_sum, > > Is the change of type of freq_sum intentional? Even with this patch, > all the callers will keep passing their frequency sums as ints and so > the implicit conversion seems a bit misleading. > > I guess complete transition to sreals would include also switch from > using cgraph_edge::frequency to sreal_frequency in > get_info_about_necessary_edges and users of struct caller_statistics. Hello. All right, I'm leaving that to you Martin ;) You are much more familiar with the code base. Martin > > I planned to work on conversions to sreals right after resolving various > exchange2 issues and I can still do this bit afterwards, but the change > in the parameter type looks like you wanted to but eventually did not? > > Thanks, > > Martin > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-24 7:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-23 12:19 [PATCH] Fix UBSAN errors in ipa-cp Martin Liška 2020-09-23 12:32 ` Jan Hubicka 2020-09-23 12:39 ` Martin Liška 2020-09-23 13:50 ` Martin Liška 2020-09-23 13:57 ` Jan Hubicka 2020-09-23 14:32 ` Martin Jambor 2020-09-23 15:02 ` Martin Jambor 2020-09-24 7:20 ` Martin Liška
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).