From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 5C6F33894C3B for ; Wed, 23 Sep 2020 13:57:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5C6F33894C3B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=hubicka@kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 2962A2829A5; Wed, 23 Sep 2020 15:57:12 +0200 (CEST) Date: Wed, 23 Sep 2020 15:57:12 +0200 From: Jan Hubicka To: Martin =?iso-8859-2?Q?Li=B9ka?= Cc: gcc-patches@gcc.gnu.org, Martin Jambor Subject: Re: [PATCH] Fix UBSAN errors in ipa-cp. Message-ID: <20200923135712.GC96998@kam.mff.cuni.cz> References: <7115ce43-bf35-c6e4-46b6-56b386e5cb34@suse.cz> <20200923123201.GB41023@kam.mff.cuni.cz> <00825d9b-10a0-b393-d044-ccbc9835c6c8@suse.cz> <752e1531-c918-89ee-5807-6c3ccce7bdf5@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <752e1531-c918-89ee-5807-6c3ccce7bdf5@suse.cz> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-15.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Sep 2020 13:57:17 -0000 > 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 > 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 *clone_num_suffixes; > @@ -498,10 +498,12 @@ ipcp_lattice::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 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::propagate_effects () > { > ipcp_value_source *src; > ipcp_value *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::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 >