Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

["bin.cheng" <bin dot cheng at linux dot alibaba dot com>]

------------------------------------------------------------------
Sender:Richard Biener <richard.guenther@gmail.com>
Sent at:2018 Oct 31 (Wed) 17:11
To:bin.cheng <bin.cheng@linux.alibaba.com>; Jan Hubicka <hubicka@ucw.cz>
Cc:GCC Patches <gcc-patches@gcc.gnu.org>
Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count
> 
> 
> On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <bin.cheng@linux.alibaba.com> 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  <bin.cheng@linux.alibaba.com>

	* profile-count.h (profile_probability::always): Add parameter.
	(profile_probability::invert): Don't invert uninitialized probability.

2018-11-02  Bin Cheng  <bin.cheng@linux.alibaba.com>

	* 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.

Attachment: 0002-Dont-invert-uninitialized-value.patch
Description: Binary data

Attachment: 0003-Check-zero-probability-count-using-member-function.patch
Description: Binary data