> -----Original Message-----
> From: Nathan Bossart <nathandbossart@gmail.com>
> Sent: Wednesday, March 13, 2024 9:39 AM
> To: Amonson, Paul D <paul.d.amonson@intel.com>
> +extern int pg_popcount32_slow(uint32 word); extern int
> +pg_popcount64_slow(uint64 word);
>
> +/* In pg_popcnt_*_accel source file. */ extern int
> +pg_popcount32_fast(uint32 word); extern int pg_popcount64_fast(uint64
> +word);
>
> Can these prototypes be moved to a header file (maybe pg_bitutils.h)? It
> looks like these are defined twice in the patch, and while I'm not positive that
> it's against project policy to declare extern function prototypes in .c files, it
> appears to be pretty rare.
Originally, I intentionally did not put these in the header file as I want them to be private, but they are not defined
inthis .c file hence extern. Now I realize the "extern" part is not needed to accomplish my goal. Will fix by removing
the"extern" keyword.
> + 'pg_popcnt_choose.c',
> + 'pg_popcnt_x86_64_accel.c',
>
> I think we want these to be architecture-specific, i.e., only built for
> x86_64 if the compiler knows how to use the relevant instructions. There is a
> good chance that we'll want to add similar support for other systems.
> The CRC32C files are probably a good reference point for how to do this.
I will look at this for the 'pg_popcnt_x86_64_accel.c' file but the 'pg_popcnt_choose.c' file is intended to be for any
platformthat may need accelerators including a possible future ARM accelerator.
> +#ifdef TRY_POPCNT_FAST
>
> IIUC this macro can be set if either 1) the popcntq test in the autoconf/meson
> scripts passes or 2) we're building with MSVC on x86_64. I wonder if it would
> be better to move the MSVC/x86_64 check to the autoconf/meson scripts so
> that we could avoid surrounding large portions of the popcount code with this
> macro. This might even be a necessary step towards building these files in an
> architecture-specific fashion.
I see the point here; however, this will take some time to get right especially since I don't have a Windows box to do
compileson. Should I attempt to do this in this patch?
Thanks,
Paul