| From: | Ewan Young <kdbase(dot)hack(at)gmail(dot)com> |
|---|---|
| To: | Michael Paquier <michael(at)paquier(dot)xyz> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, David Rowley <dgrowleyml(at)gmail(dot)com>, Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Subject: | Re: Use pg_neg_s*_overflow() for open-coded negation overflow checks |
| Date: | 2026-09-01 06:26:38 |
| Message-ID: | CAON2xHOVVyq7LGRxvXAvcFOBsfCUyY1tX096qzpNPNAJyUUfwg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue, Sep 1, 2026 at 7:45 AM Michael Paquier <michael(at)paquier(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Xuneng Zhou | 2026-09-01 06:28:19 | Re: WAIT FOR NO_THROW option could use some documentation |
| Previous Message | Virender Singla | 2026-09-01 06:18:43 | [PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple |