On Tue, Feb 21, 2023 at 11:59 AM John Naylor <[email protected]>
wrote:
>
> On Mon, Feb 20, 2023 at 10:17 PM Tom Lane <[email protected]> wrote:
> >
> > John Naylor <[email protected]> writes:
> > > Add assert checking to pg_leftmost_one_pos32() and friends
> >
> > I can see that this was worth writing for testing purposes, but
> > is it really worth carrying permanently? Even in a debug build,
> > the ratio of cycles expended to chances of finding a problem seems
> > mighty poor, and you've done a lot of damage to the readability
> > of these functions too.
>
> That's a fair point, and it's doubtful we'll need to add another platform
anytime soon. I'll work on removing the asserts.
The attached is closer to the previous coding and passes CI. I'll indent
and push this tomorrow after giving it another look, unless there is
further review.
--
John Naylor
EDB: http://www.enterprisedb.com
diff --git a/src/include/port/pg_bitutils.h b/src/include/port/pg_bitutils.h
index 9150789aaf..39b55f51ec 100644
--- a/src/include/port/pg_bitutils.h
+++ b/src/include/port/pg_bitutils.h
@@ -40,39 +40,30 @@ extern PGDLLIMPORT const uint8 pg_number_of_ones[256];
static inline int
pg_leftmost_one_pos32(uint32 word)
{
-#ifdef HAVE__BUILTIN_CLZ
- int bitscan_result;
-#elif defined(_MSC_VER)
- unsigned long bitscan_result;
+#if defined(_MSC_VER)
+ unsigned long result;
bool non_zero;
-#endif
-
-#if !defined(HAVE_BITSCAN_REVERSE) || defined(USE_ASSERT_CHECKING)
+#else
int result;
- int shift = 32 - 8;
+#endif
Assert(word != 0);
+#if defined(HAVE__BUILTIN_CLZ)
+ result = 31 - __builtin_clz(word);
+#elif defined(_MSC_VER)
+ non_zero = _BitScanReverse(&result, word);
+ Assert(non_zero);
+#else
+ int shift = 32 - 8;
+
while ((word >> shift) == 0)
shift -= 8;
result = shift + pg_leftmost_one_pos[(word >> shift) & 255];
#endif
-#ifdef HAVE_BITSCAN_REVERSE
-
-#if defined(HAVE__BUILTIN_CLZ)
- bitscan_result = 31 - __builtin_clz(word);
-#elif defined(_MSC_VER)
- non_zero = _BitScanReverse(&bitscan_result, word);
- Assert(non_zero);
-#endif
- Assert(bitscan_result == result);
- return bitscan_result;
-
-#else
return result;
-#endif /* HAVE_BITSCAN_REVERSE */
}
/*
@@ -82,46 +73,38 @@ pg_leftmost_one_pos32(uint32 word)
static inline int
pg_leftmost_one_pos64(uint64 word)
{
-#ifdef HAVE__BUILTIN_CLZ
- int bitscan_result;
-#elif defined(_MSC_VER)
- unsigned long bitscan_result;
+#if defined(_MSC_VER)
+ unsigned long result;
bool non_zero;
-#endif
-
-#if !defined(HAVE_BITSCAN_REVERSE) || defined(USE_ASSERT_CHECKING)
+#else
int result;
- int shift = 64 - 8;
-
- Assert(word != 0);
-
- while ((word >> shift) == 0)
- shift -= 8;
-
- result = shift + pg_leftmost_one_pos[(word >> shift) & 255];
#endif
-#ifdef HAVE_BITSCAN_REVERSE
+ Assert(word != 0);
#if defined(HAVE__BUILTIN_CLZ)
#if defined(HAVE_LONG_INT_64)
- bitscan_result = 63 - __builtin_clzl(word);
+ result = 63 - __builtin_clzl(word);
#elif defined(HAVE_LONG_LONG_INT_64)
- bitscan_result = 63 - __builtin_clzll(word);
+ result = 63 - __builtin_clzll(word);
#else
#error must have a working 64-bit integer datatype
#endif /* HAVE_LONG_INT_64 */
#elif defined(_MSC_VER)
- non_zero = _BitScanReverse64(&bitscan_result, word);
+ non_zero = _BitScanReverse64(&result, word);
Assert(non_zero);
+#else
+ int result;
+ int shift = 64 - 8;
+
+ while ((word >> shift) == 0)
+ shift -= 8;
+
+ result = shift + pg_leftmost_one_pos[(word >> shift) & 255];
#endif /* HAVE__BUILTIN_CLZ */
- Assert(bitscan_result == result);
- return bitscan_result;
-#else
return result;
-#endif /* HAVE_BITSCAN_REVERSE */
}
/*
@@ -132,20 +115,23 @@ pg_leftmost_one_pos64(uint64 word)
static inline int
pg_rightmost_one_pos32(uint32 word)
{
-#ifdef HAVE__BUILTIN_CTZ
- const uint32 orig_word = word;
- int bitscan_result;
-#elif defined(_MSC_VER)
- const unsigned long orig_word = word;
- unsigned long bitscan_result;
+#if defined(_MSC_VER)
+ unsigned long result;
bool non_zero;
+#else
+ int result;
#endif
-#if !defined(HAVE_BITSCAN_FORWARD) || defined(USE_ASSERT_CHECKING)
- int result = 0;
-
Assert(word != 0);
+#if defined(HAVE__BUILTIN_CTZ)
+ result = __builtin_ctz(word);
+#elif defined(_MSC_VER)
+ non_zero = _BitScanForward(&result, word);
+ Assert(non_zero);
+#else
+ result = 0;
+
while ((word & 255) == 0)
{
word >>= 8;
@@ -154,20 +140,7 @@ pg_rightmost_one_pos32(uint32 word)
result += pg_rightmost_one_pos[word & 255];
#endif
-#ifdef HAVE_BITSCAN_FORWARD
-
-#if defined(HAVE__BUILTIN_CTZ)
- bitscan_result = __builtin_ctz(orig_word);
-#elif defined(_MSC_VER)
- non_zero = _BitScanForward(&bitscan_result, orig_word);
- Assert(non_zero);
-#endif
- Assert(bitscan_result == result);
- return bitscan_result;
-
-#else
return result;
-#endif /* HAVE_BITSCAN_FORWARD */
}
/*
@@ -177,49 +150,39 @@ pg_rightmost_one_pos32(uint32 word)
static inline int
pg_rightmost_one_pos64(uint64 word)
{
-#ifdef HAVE__BUILTIN_CTZ
- const uint64 orig_word = word;
- int bitscan_result;
-#elif defined(_MSC_VER)
- const unsigned __int64 orig_word = word;
- unsigned long bitscan_result;
+#if defined(_MSC_VER)
+ unsigned long result;
bool non_zero;
+#else
+ int result;
#endif
-#if !defined(HAVE_BITSCAN_FORWARD) || defined(USE_ASSERT_CHECKING)
- int result = 0;
-
Assert(word != 0);
- while ((word & 255) == 0)
- {
- word >>= 8;
- result += 8;
- }
- result += pg_rightmost_one_pos[word & 255];
-#endif
-
-#ifdef HAVE_BITSCAN_FORWARD
-
#if defined(HAVE__BUILTIN_CTZ)
#if defined(HAVE_LONG_INT_64)
- bitscan_result = __builtin_ctzl(orig_word);
+ result = __builtin_ctzl(word);
#elif defined(HAVE_LONG_LONG_INT_64)
- bitscan_result = __builtin_ctzll(orig_word);
+ result = __builtin_ctzll(word);
#else
#error must have a working 64-bit integer datatype
#endif /* HAVE_LONG_INT_64 */
#elif defined(_MSC_VER)
- non_zero = _BitScanForward64(&bitscan_result, orig_word);
+ non_zero = _BitScanForward64(&result, word);
Assert(non_zero);
-#endif /* HAVE__BUILTIN_CTZ */
- Assert(bitscan_result == result);
- return bitscan_result;
-
#else
+ result = 0;
+
+ while ((word & 255) == 0)
+ {
+ word >>= 8;
+ result += 8;
+ }
+ result += pg_rightmost_one_pos[word & 255];
+#endif
+
return result;
-#endif /* HAVE_BITSCAN_FORWARD */
}
/*
