> -----Original Message----- > From: Nathan Bossart <nathandboss...@gmail.com> > Sent: Wednesday, March 13, 2024 9:39 AM > To: Amonson, Paul D <paul.d.amon...@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 in this .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 platform that 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 compiles on. Should I attempt to do this in this patch? Thanks, Paul
