Use pg_neg_s*_overflow() for open-coded negation overflow checks

On Thu, Aug 27, 2026 at 01:57:59PM +0800, Ewan Young wrote:

No behavioral change: each site keeps its existing hard- or soft-error
path.

-       if (value == PG_INT64_MIN)
+       if (pg_neg_s64_overflow(value, &result))

One could ask why you don't add an unlikely() here. But it looks to
me that this locaton is right and that you could just remove the
unlikely() from all the places where the overflow routines are used
anyway, because they already embed some unlikely() calls for the
minimum bound checks.

+ if (arg1 < 0 && unlikely(pg_neg_s32_overflow(arg1, &result)))
ereport(ERROR,

Well, this should embed both conditions but we don't need an unlikely
here anyway. That could also be written as the following, which seems
slightly better from here in terms of the abs functions:
if (arg1 < 0)
{
if (neg_overflow())
ereport(ERROR)
}
else
result = arg1;

- /* If the result is INT64_MIN, it cannot be represented. */

Comment removed. Perhaps it should not.

-       if (unlikely(c == PG_INT64_MIN))
+       Cash        result;

This pattern introduced in cash_div_int64() is inconsistent with the
code in cash_mul_int64() a couple of lines above.
--
Michael

Thanks for the review!

On Mon, Aug 31, 2026 at 3:26 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Aug 27, 2026 at 01:57:59PM +0800, Ewan Young wrote:

No behavioral change: each site keeps its existing hard- or soft-error
path.

-       if (value == PG_INT64_MIN)
+       if (pg_neg_s64_overflow(value, &result))

One could ask why you don't add an unlikely() here. But it looks to
me that this locaton is right and that you could just remove the
unlikely() from all the places where the overflow routines are used
anyway, because they already embed some unlikely() calls for the
minimum bound checks.

Agreed, with one refinement. The embedded unlikely() only exists in
the helpers' non-builtin fallback; on the __builtin_*_overflow path
there is no hint inside the helper. What actually makes the hint
redundant at most of these sites is that the failure branch ends in
ereport(ERROR): a constant elevel >= ERROR goes through
errstart_cold(), so the compiler already treats the branch as cold.

So in v2 this is a two-patch series:

0001 is the conversion. Converted sites whose branch raises
ereport(ERROR) use the helper bare. numericvar_to_int64() keeps its
unlikely(), because its failure branch is a plain "return false" with
no cold marking -- and the adjacent pg_mul/pg_sub_s64_overflow() calls
a few lines up in the same function keep theirs, so dropping only the
new one would trade one inconsistency for another.

0002 then removes the now-redundant unlikely() from the other
pg_{add,sub,mul}_s*_overflow() call sites in the touched files whose
branch is ereport(ERROR) -- which is, I think, the sweep your comment
was inviting. It deliberately keeps the hint where the failure branch
is ordinary code with no cold marking: the in_range() handling in
int.c/int8.c, the 128-bit fallback and numericvar_to_int64() in
numeric.c, and int4_cash()/int8_cash() (ereturn, i.e. errsave, is not
cold-marked, since the soft path can be taken routinely). I also left
the pg_neg_u* callers in numutils.c alone for the same reason (soft
goto in hot parsing code). If you'd rather see the sweep tree-wide,
or not at all, 0002 is easy to adjust or drop.

+ if (arg1 < 0 && unlikely(pg_neg_s32_overflow(arg1, &result)))
ereport(ERROR,

Well, this should embed both conditions but we don't need an unlikely
here anyway. That could also be written as the following, which seems
slightly better from here in terms of the abs functions:
if (arg1 < 0)
{
if (neg_overflow())
ereport(ERROR)
}
else
result = arg1;

Done that way in v2; it reads better than the result-preassignment
trick, agreed.

- /* If the result is INT64_MIN, it cannot be represented. */

Comment removed. Perhaps it should not.

Restored, in both int4lcm() and int8lcm().

-       if (unlikely(c == PG_INT64_MIN))
+       Cash        result;

This pattern introduced in cash_div_int64() is inconsistent with the
code in cash_mul_int64() a couple of lines above.

Fixed: cash_div_int64() now declares "Cash res" at the top and uses
the same shape as cash_mul_int64() (whose unlikely() is then removed
by 0002 along with its siblings, keeping the file uniform).

Tested: make check with and without HAVE__BUILTIN_OP_OVERFLOW, plus
manual runs of every converted function at the INT16/32/64_MIN
boundaries (unary minus, abs, division by -1, lcm, money, cash_in,
numeric->int8, and interval '... ago' with INT_MIN month/usec fields):
behavior is unchanged everywhere.

--
Michael

--
Regards,
Ewan Young

On Mon, Aug 31, 2026 at 05:46:22PM +0800, Ewan Young wrote:

0001 is the conversion. Converted sites whose branch raises
ereport(ERROR) use the helper bare. numericvar_to_int64() keeps its
unlikely(), because its failure branch is a plain "return false" with
no cold marking -- and the adjacent pg_mul/pg_sub_s64_overflow() calls
a few lines up in the same function keep theirs, so dropping only the
new one would trade one inconsistency for another.

I was looking at this one with clang and gcc, and can spot what looks
like regressions with new instructions for the following changes:
int4abs
int2abs
int4lcm
int8abs
int8clm

There are some cneg -> tbnz/tbz on arm64 and cmovnsl -> testl/jns on
x86-64, with and without the BUILTIN flag. This is telling that my
rewriting suggestion just sucks for the abs functions. Sorry. :)

Also DecodeInterval(), where itm_in() changes slightly, increasing in
activity. Not sure how to rewrite that, or if we should do it..

In all that, int4um, int4div, int42div, int8un, int8div, int84div,
int82div, cash_div_int64, cash_in, numeric_to_int64 look cleaner
overall. Without the builtin I get an identical result, and I am
seeing a variance of 0~5 less instructions with the builtin. cash_in
is showing much more reduction than the others. Note that I have kept
the unlikely() in numeric.c, you are right that this matters with
clang..

This first batch is done in the attached, as of v3 that I am planning
to apply. We could always look at the rest later, that's still a good
cut.
--
Michael

On Tue, Sep 1, 2026 at 7:45 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Aug 31, 2026 at 05:46:22PM +0800, Ewan Young wrote:

0001 is the conversion. Converted sites whose branch raises
ereport(ERROR) use the helper bare. numericvar_to_int64() keeps its
unlikely(), because its failure branch is a plain "return false" with
no cold marking -- and the adjacent pg_mul/pg_sub_s64_overflow() calls
a few lines up in the same function keep theirs, so dropping only the
new one would trade one inconsistency for another.

I was looking at this one with clang and gcc, and can spot what looks
like regressions with new instructions for the following changes:
int4abs
int2abs
int4lcm
int8abs
int8clm

There are some cneg -> tbnz/tbz on arm64 and cmovnsl -> testl/jns on
x86-64, with and without the BUILTIN flag. This is telling that my
rewriting suggestion just sucks for the abs functions. Sorry. :)

I see this went in as 8e483af5515 in the meantime -- thanks a lot for
the commit and the credit!

FWIW, the two um/div sites left out, int2um() and int2div(), turn out
to be mixed rather than clean: clang 15 compiles the int16
__builtin_sub_overflow() one instruction worse on the hot path, while
gcc 12 does one better (neg + jo).

Thanks again!

Also DecodeInterval(), where itm_in() changes slightly, increasing in
activity. Not sure how to rewrite that, or if we should do it..

In all that, int4um, int4div, int42div, int8un, int8div, int84div,
int82div, cash_div_int64, cash_in, numeric_to_int64 look cleaner
overall. Without the builtin I get an identical result, and I am
seeing a variance of 0~5 less instructions with the builtin. cash_in
is showing much more reduction than the others. Note that I have kept
the unlikely() in numeric.c, you are right that this matters with
clang..

This first batch is done in the attached, as of v3 that I am planning
to apply. We could always look at the rest later, that's still a good
cut.
--
Michael

--
Regards,
Ewan Young

On Tue, Sep 01, 2026 at 02:26:38PM +0800, Ewan Young wrote:

I see this went in as 8e483af5515 in the meantime -- thanks a lot for
the commit and the credit!

FWIW, the two um/div sites left out, int2um() and int2div(), turn out
to be mixed rather than clean: clang 15 compiles the int16
__builtin_sub_overflow() one instruction worse on the hot path, while
gcc 12 does one better (neg + jo).

Please note that I'd be OK to look at this stuff again if we have
other similar opportunities. I am not really convinced by your
v2-0002. The other opportunities in v2-0001 could be reconsidered if
rewritten in a different way, but it's also tricky to evaluate due to
the with/without builtin and compiler requirements.

I would accept something that proves to be a net benefit in rather new
versions of gcc and clang, as well as a net benefit with/without the
builtin. 8e483af5515 was exactly that, with less or the same amount of
instructions (the cash change was super nice) for all setups I have
tested (Linux and macos).
--
Michael

On Wed, Sep 2, 2026 at 7:16 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Sep 01, 2026 at 02:26:38PM +0800, Ewan Young wrote:

I see this went in as 8e483af5515 in the meantime -- thanks a lot for
the commit and the credit!

FWIW, the two um/div sites left out, int2um() and int2div(), turn out
to be mixed rather than clean: clang 15 compiles the int16
__builtin_sub_overflow() one instruction worse on the hot path, while
gcc 12 does one better (neg + jo).

Please note that I'd be OK to look at this stuff again if we have
other similar opportunities. I am not really convinced by your
v2-0002. The other opportunities in v2-0001 could be reconsidered if
rewritten in a different way, but it's also tricky to evaluate due to
the with/without builtin and compiler requirements.

I would accept something that proves to be a net benefit in rather new
versions of gcc and clang, as well as a net benefit with/without the
builtin. 8e483af5515 was exactly that, with less or the same amount of
instructions (the cash change was super nice) for all setups I have
tested (Linux and macos).

Fair enough on 0002 -- let's drop it. Thanks again for taking the good batch!

--
Michael

--
Regards,
Ewan Young