> On 11 Aug 2026, at 08:31, Richard Biener <[email protected]> wrote: > > On Mon, Aug 10, 2026 at 4:28 PM <[email protected]> wrote: >> >> From: Kyrylo Tkachov <[email protected]> >> >> profile_probability::apply uses RDIV, which adds half the denominator >> before division. This does not round negative values correctly. >> Multiplying the signed input by the fixed-point probability can also >> overflow for large inputs. >> >> Scale the unsigned magnitude with safe_scale_64bit, then restore the sign. >> Return the input directly for a unit probability so that the minimum >> gcov_type value remains representable. Preserve the existing truncation >> for uninitialized probabilities. >> >> Add selftests for signed rounding, zero and unit probabilities, >> uninitialized probabilities, and the minimum and maximum gcov_type values. >> >> Bootstrapped and tested on aarch64-none-linux and x86_64-linux. >> Ok for trunk? > > So the reason I have not yet acked this is that I wonder why we'd have > negative probabilities at all? To me that doesn't make sense.
There are no negative probabilities here. profile_probability remains in the range from zero to one. The signed quantity is the gcov_type val argument to profile_probability::apply. Before patch 2, ifcvt calls it with an arm-cost delta: e->probability.apply ((signed) (then_cost - else_cost)) The intended value is negative when then_cost is less than else_cost. For example, applying a probability of one half to -3 gives -1 with the old RDIV expression, instead of -2 with symmetric nearest rounding. The signed multiplication can also overflow for large value arguments. This patch keeps the probability unsigned and in range. It scales the unsigned magnitude of val and then restores the sign. Patch 2 changes average_cost to pass a nonnegative delta. That is a separate change which makes the rounded cost independent of CFG arm order. Thanks, Kyrill > >> Thanks, >> Kyrill >> >> gcc/ChangeLog: >> >> * profile-count.h (profile_probability::apply): Scale an unsigned >> magnitude with safe_scale_64bit. >> * profile-count.cc (test_profile_probability_apply): New. >> (profile_count_cc_tests): New. >> * selftest.h (profile_count_cc_tests): Declare. >> * selftest-run-tests.cc (selftest::run_tests): Run it. >> >> Signed-off-by: Kyrylo Tkachov <[email protected]> >> --- >> gcc/profile-count.cc | 47 +++++++++++++++++++++++++++++++++++++++ >> gcc/profile-count.h | 26 +++++++++++++++++++++- >> gcc/selftest-run-tests.cc | 1 + >> gcc/selftest.h | 1 + >> 4 files changed, 74 insertions(+), 1 deletion(-) >> >> diff --git a/gcc/profile-count.cc b/gcc/profile-count.cc >> index 2cce9caa772..9be209efda2 100644 >> --- a/gcc/profile-count.cc >> +++ b/gcc/profile-count.cc >> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see >> #include "wide-int.h" >> #include "sreal.h" >> #include "profile.h" >> +#include "selftest.h" >> >> /* Names from profile_quality enum values. */ >> >> @@ -595,3 +596,49 @@ profile_count::force_nonzero () const >> } >> return ret; >> } >> + >> +#if CHECKING_P >> + >> +namespace selftest { >> + >> +/* Verify profile_probability::apply. */ >> + >> +static void >> +test_profile_probability_apply () >> +{ >> + const gcov_type min = INTTYPE_MINIMUM (gcov_type); >> + const gcov_type max = INTTYPE_MAXIMUM (gcov_type); >> + profile_probability quarter = profile_probability::guessed_always () / 4; >> + profile_probability even = profile_probability::even (); >> + >> + ASSERT_EQ (1, quarter.apply (3)); >> + ASSERT_EQ (-1, quarter.apply (-3)); >> + ASSERT_EQ (2, even.apply (3)); >> + ASSERT_EQ (-2, even.apply (-3)); >> + ASSERT_EQ (0, even.apply (0)); >> + >> + ASSERT_EQ (0, profile_probability::never ().apply (min)); >> + ASSERT_EQ (min, profile_probability::always ().apply (min)); >> + ASSERT_EQ (max, profile_probability::always ().apply (max)); >> + ASSERT_EQ (min, profile_probability::guessed_always ().apply (min)); >> + ASSERT_EQ (min / 2, even.apply (min)); >> + ASSERT_EQ (max / 2 + 1, even.apply (max)); >> + >> + profile_probability uninitialized = profile_probability::uninitialized (); >> + ASSERT_EQ (1, uninitialized.apply (3)); >> + ASSERT_EQ (-1, uninitialized.apply (-3)); >> + ASSERT_EQ (min / 2, uninitialized.apply (min)); >> + ASSERT_EQ (max / 2, uninitialized.apply (max)); >> +} >> + >> +/* Run all of the selftests within this file. */ >> + >> +void >> +profile_count_cc_tests () >> +{ >> + test_profile_probability_apply (); >> +} >> + >> +} // namespace selftest >> + >> +#endif >> diff --git a/gcc/profile-count.h b/gcc/profile-count.h >> index b424ecba3ea..76325d0fdfa 100644 >> --- a/gcc/profile-count.h >> +++ b/gcc/profile-count.h >> @@ -513,11 +513,35 @@ public: >> return ret; >> } >> >> + /* Return VAL scaled by this probability. Round initialized probabilities >> + to the nearest integer, with halfway values away from zero. Treat an >> + uninitialized probability as one half and truncate toward zero. */ >> gcov_type apply (gcov_type val) const >> { >> if (*this == uninitialized ()) >> return val / 2; >> - return RDIV (val * m_val, max_probability); >> + >> + /* A unit probability leaves VAL unchanged. Return it directly >> because >> + the magnitude of the minimum gcov_type value is one greater than the >> + maximum gcov_type value. */ >> + if (m_val == max_probability) >> + return val; >> + >> + /* Convert to unsigned before negating so that the minimum gcov_type >> + value has a representable magnitude. Scale the magnitude with >> + overflow-safe arithmetic, then restore the sign. */ >> + gcov_type_unsigned magnitude >> + = val < 0 ? -(gcov_type_unsigned) val : (gcov_type_unsigned) val; >> + uint64_t scaled; >> + bool scaled_p >> + = safe_scale_64bit (magnitude, m_val, max_probability, &scaled); >> + /* The scaled result fits in uint64_t. With the unit case handled >> above, >> + it also fits in the nonnegative range of gcov_type. */ >> + gcc_checking_assert (scaled_p); >> + gcc_checking_assert >> + (scaled <= (gcov_type_unsigned) INTTYPE_MAXIMUM (gcov_type)); >> + >> + return val < 0 ? -(gcov_type) scaled : (gcov_type) scaled; >> } >> >> /* Return 1-*THIS. */ >> diff --git a/gcc/selftest-run-tests.cc b/gcc/selftest-run-tests.cc >> index 1d21b8751b4..07eb984e918 100644 >> --- a/gcc/selftest-run-tests.cc >> +++ b/gcc/selftest-run-tests.cc >> @@ -72,6 +72,7 @@ selftest::run_tests () >> wide_int_cc_tests (); >> ggc_tests_cc_tests (); >> sreal_cc_tests (); >> + profile_count_cc_tests (); >> fibonacci_heap_cc_tests (); >> typed_splay_tree_cc_tests (); >> opt_suggestions_cc_tests (); >> diff --git a/gcc/selftest.h b/gcc/selftest.h >> index f2e2b49fc20..acfc4a46e49 100644 >> --- a/gcc/selftest.h >> +++ b/gcc/selftest.h >> @@ -248,6 +248,7 @@ extern void path_coverage_cc_tests (); >> extern void predict_cc_tests (); >> extern void pretty_print_cc_tests (); >> extern void pretty_print_token_buffer_cc_tests (); >> +extern void profile_count_cc_tests (); >> extern void pub_sub_cc_tests (); >> extern void range_op_tests (); >> extern void range_tests (); >> -- >> 2.50.1 (Apple Git-155) >>
