Commit 88ea7a11 authored by Noah Misch's avatar Noah Misch

Choose ppc compare_exchange constant path for more operand values.

The implementation uses smaller code when the "expected" operand is a
small constant, but the implementation needlessly defined the set of
acceptable constants more narrowly than the ABI does.  Core PostgreSQL
and PGXN don't use the constant path at all, so this is future-proofing.
Back-patch to v13, where commit 30ee5d17
introduced this code.

Reviewed by Tom Lane.  Reported by Christoph Berg.

Discussion: https://postgr.es/m/20201009092825.GD889580@msg.df7cb.de
parent f5c1167b
...@@ -72,14 +72,6 @@ typedef struct pg_atomic_uint64 ...@@ -72,14 +72,6 @@ typedef struct pg_atomic_uint64
* the __asm__. (That would remove the freedom to eliminate dead stores when * the __asm__. (That would remove the freedom to eliminate dead stores when
* the caller ignores "expected", but few callers do.) * the caller ignores "expected", but few callers do.)
* *
* The cmpwi variant may be dead code. In gcc 7.2.0,
* __builtin_constant_p(*expected) always reports false.
* __atomic_compare_exchange_n() does use cmpwi when its second argument
* points to a constant. Hence, using this instead of
* __atomic_compare_exchange_n() nominally penalizes the generic.h
* pg_atomic_test_set_flag_impl(). Modern GCC will use the generic-gcc.h
* version, making the penalty theoretical only.
*
* Recognizing constant "newval" would be superfluous, because there's no * Recognizing constant "newval" would be superfluous, because there's no
* immediate-operand version of stwcx. * immediate-operand version of stwcx.
*/ */
...@@ -94,7 +86,8 @@ pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr, ...@@ -94,7 +86,8 @@ pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P #ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) && if (__builtin_constant_p(*expected) &&
*expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN) (int32) *expected <= PG_INT16_MAX &&
(int32) *expected >= PG_INT16_MIN)
__asm__ __volatile__( __asm__ __volatile__(
" sync \n" " sync \n"
" lwarx %0,0,%5 \n" " lwarx %0,0,%5 \n"
...@@ -183,7 +176,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, ...@@ -183,7 +176,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
/* Like u32, but s/lwarx/ldarx/; s/stwcx/stdcx/; s/cmpw/cmpd/ */ /* Like u32, but s/lwarx/ldarx/; s/stwcx/stdcx/; s/cmpw/cmpd/ */
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P #ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) && if (__builtin_constant_p(*expected) &&
*expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN) (int64) *expected <= PG_INT16_MAX &&
(int64) *expected >= PG_INT16_MIN)
__asm__ __volatile__( __asm__ __volatile__(
" sync \n" " sync \n"
" ldarx %0,0,%5 \n" " ldarx %0,0,%5 \n"
......
...@@ -720,6 +720,14 @@ test_atomic_uint32(void) ...@@ -720,6 +720,14 @@ test_atomic_uint32(void)
EXPECT_EQ_U32(pg_atomic_read_u32(&var), (uint32) INT_MAX + 1); EXPECT_EQ_U32(pg_atomic_read_u32(&var), (uint32) INT_MAX + 1);
EXPECT_EQ_U32(pg_atomic_sub_fetch_u32(&var, INT_MAX), 1); EXPECT_EQ_U32(pg_atomic_sub_fetch_u32(&var, INT_MAX), 1);
pg_atomic_sub_fetch_u32(&var, 1); pg_atomic_sub_fetch_u32(&var, 1);
expected = PG_INT16_MAX;
EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
expected = PG_INT16_MAX + 1;
EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
expected = PG_INT16_MIN;
EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
expected = PG_INT16_MIN - 1;
EXPECT_TRUE(!pg_atomic_compare_exchange_u32(&var, &expected, 1));
/* fail exchange because of old expected */ /* fail exchange because of old expected */
expected = 10; expected = 10;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment