Обсуждение: pgsql: For all ppc compilers,implement pg_atomic_fetch_add_ with inlin

Поиск
Список
Период
Сортировка

pgsql: For all ppc compilers,implement pg_atomic_fetch_add_ with inlin

От
Noah Misch
Дата:
For all ppc compilers, implement pg_atomic_fetch_add_ with inline asm.

This is more like how we handle s_lock.h and arch-x86.h.  This does not
materially affect code generation for gcc 7.2.0 or xlc 13.1.3.

Reviewed by Tom Lane.

Discussion: https://postgr.es/m/20190831071157.GA3251746@rfd.leadboat.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/e7ff59686eacf5021fb84be921116986c3828d8a

Modified Files
--------------
configure                              | 40 ++++++++++++++
configure.in                           | 20 +++++++
src/include/pg_config.h.in             |  3 ++
src/include/port/atomics/arch-ppc.h    | 98 ++++++++++++++++++++++++++++++++++
src/include/port/atomics/generic-xlc.h | 66 -----------------------
5 files changed, 161 insertions(+), 66 deletions(-)


Re: pgsql: For all ppc compilers, implement pg_atomic_fetch_add_ with inlin

От
Tom Lane
Дата:
Noah Misch <noah@leadboat.com> writes:
> For all ppc compilers, implement pg_atomic_fetch_add_ with inline asm.

prairiedog thinks there's something wrong here --- probably, some global
enabling #define is missing?

I can't investigate right now, maybe later today.

            regards, tom lane



Re: pgsql: For all ppc compilers, implement pg_atomic_fetch_add_with inlin

От
Noah Misch
Дата:
On Sat, Sep 14, 2019 at 10:10:47AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > For all ppc compilers, implement pg_atomic_fetch_add_ with inline asm.
> 
> prairiedog thinks there's something wrong here --- probably, some global
> enabling #define is missing?

I can reproduce its situation by commenting out the HAVE_GCC_ symbols in
pg_config.h.  This commit did "#define PG_HAVE_ATOMIC_U32_SUPPORT" but defined
only fetch_add, not compare_exchange.  That works with generic-gcc.h, but
fallback.h can't replace just one.  (Rightly so -- fallback.h defines the
pg_atomic_uint32 structure differently.)  I can see these ways to fix this:

1. Add pg_atomic_compare_exchange_u{32,64}_impl to arch-ppc.h
2. Arrange for arch-ppc.h to be a no-op when GCC atomics are unavailable
3. Just revert

(1) seems best.  Since it may be days or weeks before I get to that, I'm
inclined to revert after ~24h of total buildfarm exposure.



Re: pgsql: For all ppc compilers, implement pg_atomic_fetch_add_ with inlin

От
Tom Lane
Дата:
Noah Misch <noah@leadboat.com> writes:
> On Sat, Sep 14, 2019 at 10:10:47AM -0400, Tom Lane wrote:
>> prairiedog thinks there's something wrong here --- probably, some global
>> enabling #define is missing?

> I can reproduce its situation by commenting out the HAVE_GCC_ symbols in
> pg_config.h.  This commit did "#define PG_HAVE_ATOMIC_U32_SUPPORT" but defined
> only fetch_add, not compare_exchange.  That works with generic-gcc.h, but
> fallback.h can't replace just one.  (Rightly so -- fallback.h defines the
> pg_atomic_uint32 structure differently.)

Agreed on that diagnosis.

> I can see these ways to fix this:
> 1. Add pg_atomic_compare_exchange_u{32,64}_impl to arch-ppc.h
> 2. Arrange for arch-ppc.h to be a no-op when GCC atomics are unavailable
> 3. Just revert
> (1) seems best.  Since it may be days or weeks before I get to that, I'm
> inclined to revert after ~24h of total buildfarm exposure.

Yeah, seems like it should be possible to build atomic_compare_exchange
as an LWARX/STWCX loop, but it will take a little work.

I concur with waiting till your AIX flotilla has checked in before
reverting.  One thing we could find out that way is whether we actually
need the added configure test.  The returns so far say we don't:

 cavefish      | 2019-09-14 04:38:49 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 chub          | 2019-09-14 15:10:02 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 clam          | 2019-09-14 09:30:04 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 cuon          | 2019-09-14 06:57:23 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 demoiselle    | 2019-09-14 15:15:45 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 devario       | 2019-09-14 14:25:21 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 dhole         | 2019-09-14 05:46:33 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 elasmobranch  | 2019-09-14 03:18:50 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 gadwall       | 2019-09-14 12:39:48 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 prairiedog    | 2019-09-14 08:12:53 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 quokka        | 2019-09-14 08:46:25 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 shoveler      | 2019-09-14 14:44:30 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 takin         | 2019-09-14 08:49:27 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 tern          | 2019-09-14 09:51:04 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 urocryon      | 2019-09-14 06:45:31 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes
 vulpes        | 2019-09-14 09:35:41 | checking whether __builtin_constant_p(x) implies "i"(x) acceptance... yes

If the other AIX critters agree with tern, I'd be inclined to drop the
configure test and just make the code check for HAVE__BUILTIN_CONSTANT_P.

            regards, tom lane