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