Re: [PATCH 1/3] profile-count: Handle signed values in profile_probability::apply

On 8/11/2026 2:38 AM, Kyrylo Tkachov wrote:
>
>> 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.
I think a bit more context may help here.  If we look at average_cost:

/* Compute average of two given costs weighted by relative probabilities
    of respective basic blocks in an IF-THEN-ELSE.  E is the IF-THEN edge.
    With P as the probability to take the IF-THEN branch, return
    P * THEN_COST + (1 - P) * ELSE_COST.  */
static unsigned
average_cost (unsigned then_cost, unsigned else_cost, edge e)
{
   return else_cost + e->probability.apply ((signed) (then_cost - 
else_cost));
}

Of course this gets rewritten in patch #2.  So it's less of an issue.  
Though this does show a mostly reasonable way to use the APIs that can 
result in a negative value being passed in and adjusts the code to 
handle it more correctly.

Jeff