From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Greg Stark <stark(at)mit(dot)edu> |
Subject: | Re: Current int & float overflow checking is slow. |
Date: | 2017-10-30 17:54:45 |
Message-ID: | 20171030175445.uayyn7ieptl4hmby@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2017-10-30 22:29:42 +0530, Robert Haas wrote:
> On Mon, Oct 30, 2017 at 4:57 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > 0001) Introduces pg_{add,sub,mul}{16,32,64}_overflow(a, b, *result)
> > These use compiler intrinsics on gcc/clang. If that's not
> > available, they cast to a wider type and to overflow checks. For
> > 64bit there's a fallback for the case 128bit math is not
> > available (here I stole from an old patch of Greg's).
> >
> > These fallbacks are, as far as I can tell, C free of overflow
> > related undefined behaviour.
>
> Looks nice.
Thanks.
> > Perhaps it should rather be pg_add_s32_overflow, or a similar
> > naming scheme?
>
> Not sure what the s is supposed to be? Signed?
Yes, signed. So we could add a u32 or something complementing the
functions already in the patch. Even though overflow checks are a heck
of a lot easier to write for unsigned ints, the intrinsics are still
faster. I don't have any sort of strong feelings on the naming.
> > 0002) Converts int.c, int8.c and a smattering of other functions to use
> > the new facilities. This removes a fair amount of code.
> >
> > It might make sense to split this up further, but right now that's
> > the set of functions that either are affected performancewise by
> > previous overflow checks, and/or relied on wraparound
> > overflow. There's probably more places, but this is what I found
> > by visual inspection and compiler warnings.
>
> I lack the patience to review this tonight.
Understandable ;)
> > 0003) Removes -fwrapv. I'm *NOT* suggesting we apply this right now, but
> > it seems like an important test for the new facilities. Without
> > 0002, tests would fail after this, after it all tests run
> > successfully.
>
> I suggest that if we think we don't need -fwrapv any more, we ought to
> remove it. Otherwise, we won't find out if we're wrong.
I agree that we should do so at some point not too far away in the
future. Not the least because we don't specify this kind of C dialect in
a lot of other compilers. Additionally the flag causes some slowdown
(because e.g. for loop variables are optimized less). But I'm fairly
certain it needs a bit more care that I've invested as of now - should
probably at least compile with -Wstrict-overflow=some-higher-level, and
with ubsan. I'm fairly certain there's more bogus overflow checks
around...
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2017-10-30 17:58:43 | Re: Remove secondary checkpoint |
Previous Message | Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?= | 2017-10-30 17:38:54 | [PATCH] Comment typo in get_collation_name() comment |