------------------------------------------------------------------ Sender:Richard Biener Sent at:2018 Oct 31 (Wed) 17:11 To:bin.cheng ; Jan Hubicka Cc:GCC Patches Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count > > > On Wed, Oct 31, 2018 at 7:30 AM bin.cheng wrote: >> >> Hi, >> In new profile probability/count infra, we have different precision quality categories, >> and probabilities/counts of different categories are not supposed to be compared or >> calculated. Though in general is an improvement, it introduces unexpected behavior. >> Specifically, class profile_probablity and profile_count themselves are implemented >> by comparing probabilities/counts against profile_count::zero(). while zero() is of >> profile_precision category, it's always compared different to zero of other precision >> categories including afdo. >> >> I can see two ways fixing this: 1) Treat zero as a common probability/count regardless >> of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison >> against probability_count::zero(). 2) requires lots of code changes so I went with 1) >> in this patch set. This patch doesn't handle "always" but it might be. >> >> This patch also corrects a minor issue where we try to invert an uninitialized value. >> >> Bootstrap and test on x86_64 in patch set. Is it OK? > Thanks for all the reviews. > I'll defer on the emit_store_flag_force change, likewise for the zero As Jeff pointed out too, this is discarded now. > handling in > compares - I don't think zeros of different qualities should compare equal. > Would compares against ::always() not have the very same issue? > Likewise ::even(), > ::likely(), etc.? Those always get guessed quality. In this version, I went with the second method which adds never_p/zero_p member function for probability and count. Check on ZERO value won't fail unexpected now. The patch also makes some other changes that avoids short-circuit returning on ZERO probability/count and let. > > The invert change looks OK to me. The related change to the always() API would > suggest to replace guessed_always() with always (guessed) and also do similar > changes throughout the whole API... The invert part is split as a standalone patch. I think adding a default precision parameter to member functions is a little bit better than having various version functions here, like you mentioned for from_gcov_type. Bootstrap and test on x86_64 in patch set. It fixes all (13) ICE autofdo tests. Thanks, bin 2018-11-02 Bin Cheng * profile-count.h (profile_probability::always): Add parameter. (profile_probability::invert): Don't invert uninitialized probability. 2018-11-02 Bin Cheng * profile-count.h (profile_probability::never_p): New. (profile_probability::operator+, +=, -, -=, *, *=, /, /=): Check ZERO probability using never_p. (profile_probability::apply_scale): Likewise. (profile_probability::apply): Check using uninitialized_p. (profile_count::zero_p): New. (profile_count::compatible_p): Check ZERO count using zero_p. (profile_count::operator+, +=, <, >, <=, >=, apply_scale): Likewise. (profile_count::apply_probability, probability_in): Likewise. (profile_count::operator-, -=): Likewise. Don't quick return if RHS profile_count is ZERO. (profile_count::max): Don't quick return in case of ZERO count. * profile-count.c (profile_count::to_frequency): Check ZERO profile probability or count using never_p or zero_p. (profile_count::to_cgraph_frequency, to_sreal_scale): Likewise. (profile_count::adjust_for_ipa_scaling): Likewise. (profile_count::combine_with_ipa_count): Likewise. (profile_probability::combine_with_count): Likewise. * bb-reorder.c (sanitize_hot_paths): Likewise. * cfg.c (update_bb_profile_for_threading): Likewise. (scale_bbs_frequencies_profile_count): Likewise. * ipa-profile.c (ipa_propagate_frequency_1): Likewise. (ipa_propagate_frequency): Likewise. * ipa-utility.c (ipa_merge_profiles): Likewise. * predict.c (maybe_hot_count_p, probably_never_executed): Likewise. (probably_never_executed_bb_p, unlikely_executed_stmt_p): Likewise. (combine_predictions_for_bb): Likewise. (drop_profile, handle_missing_profiles): Likewise. (propagate_unlikely_bbs_forward): Likewise. (determine_unlikely_bbs): Likewise. (estimate_bb_frequencies): Likewise. (compute_function_frequency, force_edge_cold): Likewise. * profile.c (compute_branch_probabilities): Likewise. * shrink-wrap.c (try_shrink_wrapping): Likewise. * tree-ssa-loop-manip.c (scale_dominated_blocks_in_loop): Likewise. (tree_transform_and_unroll_loop): Likewise. * tree-ssa-threadupdate.c (update_profile): Likewise. * tree-vect-loop.c (scale_profile_for_vect_loop): Likewise.