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
