On Tue, Mar 26, 2024 at 09:48:57PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandboss...@gmail.com> writes:
>> I just did the minimal fix for now, i.e., I moved the new label into the
>> SIMD section of the function. I think it would be better stylistically to
>> move the one-by-one logic to an inline helper function, but I didn't do
>> that just in case it might negatively impact performance. I'll look into
>> this and will follow up with another patch if it looks good.
>
> Sounds like a plan.
Here's what I had in mind. My usual benchmark seems to indicate that this
shouldn't impact performance.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From c3f163753246c5ec82dd8c5dba70232cbeebbf2a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Wed, 27 Mar 2024 13:50:17 -0500
Subject: [PATCH v1 1/1] improve style of pg_lfind32()
---
src/include/port/pg_lfind.h | 58 +++++++++++++++----------------------
1 file changed, 24 insertions(+), 34 deletions(-)
diff --git a/src/include/port/pg_lfind.h b/src/include/port/pg_lfind.h
index 33e8471b03..5b76cc8937 100644
--- a/src/include/port/pg_lfind.h
+++ b/src/include/port/pg_lfind.h
@@ -80,6 +80,24 @@ pg_lfind8_le(uint8 key, uint8 *base, uint32 nelem)
return false;
}
+/*
+ * pg_lfind32_one_by_one_helper
+ *
+ * Searches the array of integers one-by-one. The caller is responsible for
+ * ensuring that there are at least "nelem" integers in the array.
+ */
+static inline bool
+pg_lfind32_one_by_one_helper(uint32 key, uint32 *base, uint32 nelem)
+{
+ for (int i = 0; i < nelem; i++)
+ {
+ if (key == base[i])
+ return true;
+ }
+
+ return false;
+}
+
#ifndef USE_NO_SIMD
/*
* pg_lfind32_simd_helper
@@ -134,9 +152,8 @@ pg_lfind32_simd_helper(const Vector32 keys, uint32 *base)
static inline bool
pg_lfind32(uint32 key, uint32 *base, uint32 nelem)
{
- uint32 i = 0;
-
#ifndef USE_NO_SIMD
+ uint32 i = 0;
/*
* For better instruction-level parallelism, each loop iteration operates
@@ -150,25 +167,15 @@ pg_lfind32(uint32 key, uint32 *base, uint32 nelem)
const uint32 tail_idx = nelem & ~(nelem_per_iteration - 1);
#if defined(USE_ASSERT_CHECKING)
- bool assert_result = false;
-
- /* pre-compute the result for assert checking */
- for (int j = 0; j < nelem; j++)
- {
- if (key == base[j])
- {
- assert_result = true;
- break;
- }
- }
+ bool assert_result = pg_lfind32_one_by_one_helper(key, base, nelem);
#endif
/*
- * If there aren't enough elements for the SIMD code, jump to the standard
+ * If there aren't enough elements for the SIMD code, use the standard
* one-by-one linear search code.
*/
if (nelem < nelem_per_iteration)
- goto one_by_one;
+ return pg_lfind32_one_by_one_helper(key, base, nelem);
/*
* Process as many elements as possible with a block of 4 registers.
@@ -193,27 +200,10 @@ pg_lfind32(uint32 key, uint32 *base, uint32 nelem)
*/
Assert(assert_result == pg_lfind32_simd_helper(keys, &base[nelem - nelem_per_iteration]));
return pg_lfind32_simd_helper(keys, &base[nelem - nelem_per_iteration]);
-
-one_by_one:
-
-#endif /* ! USE_NO_SIMD */
-
+#else
/* Process the elements one at a time. */
- for (; i < nelem; i++)
- {
- if (key == base[i])
- {
-#ifndef USE_NO_SIMD
- Assert(assert_result == true);
+ return pg_lfind32_one_by_one_helper(key, base, nelem);
#endif
- return true;
- }
- }
-
-#ifndef USE_NO_SIMD
- Assert(assert_result == false);
-#endif
- return false;
}
#endif /* PG_LFIND_H */
--
2.25.1
