Date: Thu, 15 Jan 2026 09:59:17 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: John Naylor <johncnaylorls@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: refactor architecture-specific popcount code
Message-ID: <aWkO1TG5gWLhUtAJ@nathan>
References: <aWf_InS1VrbeXAfP@nathan>
 <CANWCAZb0U2qDTHCxPDyZugOTSE7xPURWKwcf8d4JmYTgH4+nEA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CANWCAZb0U2qDTHCxPDyZugOTSE7xPURWKwcf8d4JmYTgH4+nEA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aWkO1TG5gWLhUtAJ%40nathan>
Precedence: bulk

On Thu, Jan 15, 2026 at 04:07:51PM +0700, John Naylor wrote:
> Thanks for taking on some technical debt!

Thanks for reviewing.

> --- a/src/port/pg_popcount_avx512.c
> +++ b/src/port/pg_popcount_x86_64.c
> 
> Can we get away with just "x86" for brevity? We generally don't target
> 32-bit CPUs for this kind of work, so there's no chance of confusion.

WFM.

> ```
> -#ifdef USE_AVX512_POPCNT_WITH_RUNTIME_CHECK
> +#include "port/pg_bitutils.h"
> +
> +#ifdef TRY_POPCNT_X86_64
> 
>  #if defined(HAVE__GET_CPUID) || defined(HAVE__GET_CPUID_COUNT)
>  #include <cpuid.h>
>  #endif
> ```
> 
> With the above in the x86 .c file, I wonder we can get rid of this
> stanza and the "try" symbol and gate only on HAVE_X86_64_POPCNTQ:
> 
> #ifdef HAVE_X86_64_POPCNTQ
> #if defined(HAVE__GET_CPUID) || defined(HAVE__CPUID)
> #define TRY_POPCNT_X86_64 1
> #endif
> #endif
> 
> If we have to be cautious, we could just turn the #error on no CPUID
> symbol into "return false".

Yeah, the CPUID macro checks do seem overly cautious to me, especially
since we've just #error'd when the CPUID intrinsics are missing in
pg_crc32c_sse42_choose.c since 2015.  That seems to suggest that nobody is
trying to build Postgres with a compiler that knows about SSE4.2/POPCNT but
not CPUID.  For reference, CPUID was introduced in 1993.

I bet we could also convert this bit into a configuration-time check:

	#if defined(_MSC_VER) && defined(_M_AMD64)
	#define HAVE_X86_64_POPCNTQ
	#endif

> s/fast/sse42/:
> 
> Seems okay in this file, but this isn't the best name, either. Maybe a
> comment to head off future "corrections", something like:
> "Technically, POPCNT is not part of SSE 4.2, and is not even a vector
> operation, but many compilers emit the popcnt instruction with
> -msse4.2 anyway."

Makes sense.

-- 
nathan