Hi,
On 2020-02-13 16:25:25 +0000, Emre Hasegeli wrote:
> And also this commit is changing the usage of unlikely() to cover
> the whole condition. Using it only for the result is not semantically
> correct. It is more than likely for the result to be infinite when
> the input is, or it to be 0 when the input is.
I'm not really convinced by this fwiw.
Comparing
if (unlikely(isinf(result) && !isinf(num)))
float_overflow_error();
with
if (unlikely(isinf(result)) && !isinf(num))
float_overflow_error();
I don't think it's clear that we want the former. What we want to
express is that it's unlikely that the result is infinite, and that the
compiler should optimize for that. Since there's a jump involved between
the check for isinf(result) and the one for !isinf(num), we want the
compiler to implement this so the non-overflow path follows the first
check, and the rest of the check is later.
> +void float_overflow_error()
> +{
Tom's probably on this, but it should be (void).
> @@ -2846,23 +2909,21 @@ float8_accum(PG_FUNCTION_ARGS)
>
> /*
> * Overflow check. We only report an overflow error when finite
> * inputs lead to infinite results. Note also that Sxx should be NaN
> * if any of the inputs are infinite, so we intentionally prevent Sxx
> * from becoming infinite.
> */
> if (isinf(Sx) || isinf(Sxx))
> {
> if (!isinf(transvalues[1]) && !isinf(newval))
> - ereport(ERROR,
> - (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> - errmsg("value out of range: overflow")));
> + float_overflow_error();
>
> Sxx = get_float8_nan();
> }
> }
Probably worth unifiying the use of unlikely around isinf here and in
the follow functions.
Greetings,
Andres Freund
