pgsql: For all ppc compilers, implement pg_atomic_fetch_add_ with inlin

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

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.

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