Thanks for looking at this. On Fri, 1 Feb 2019 at 11:45, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > > I only have cosmetic suggestions for this patch. For one thing, I think > the .c file should be in src/port and its header should be in > src/include/port/, right beside the likes of pg_bswap.h and pg_crc32c.h.
I've moved the code into src/port and renamed the file to pg_bitutils.c > For another, I think the arrangement of all those "ifdef > HAVE_THIS_OR_THAT" in the bitutils.c file is a bit hard to read. I'd > lay them out like this: I've made this change too, although when doing it I realised that I had forgotten to include the check for CPUID. It's possible that does not exist but POPCNT does, I guess. This has made the #ifs a bit more complex. > Finally, the part in bitmapset.c is repetitive on the #ifdefs; I'd just > put at the top of the file something like Yeah, agreed. Much neater that way. > Other than those minor changes, I think we should just get this pushed > and see what the buildfarm thinks. In the words of a famous PG hacker: > if a platform ain't in the buildfarm, we don't support it. I also made a number of other changes to the patch. 1. The patch now only uses the -mpopcnt CFLAG for pg_bitutils.c. I thought this was important so we don't expose that flag in pg_config and possibly end up building extension with popcnt instructions, which might not be portable to other older hardware. 2. Wrote a new pg_popcnt function that accepts an array of bytes and a size variable. This seems useful for the bloomfilter use. There are still various number_of_ones[] arrays around the codebase. These exist in tsgistidx.c, _intbig_gist.c and _ltree_gist.c. It would be nice to get rid of those too, but one of the usages in each of those 3 files requires XORing with another bit array before counting the bits. I thought about maybe writing a pop_count_xor() function that accepts 2 byte arrays and a length parameter, but it seems a bit special case, so I didn't. Another thing I wasn't sure of was if I should just have bms_num_members() just call pg_popcount(). It might be worth benchmarking to see what's faster. My thinking is that pg_popcount will inline the pg_popcount64() call so it would mean a single function call rather than one for each bitmapword in the set. I've compiled and run make check-world on Linux with GCC7.3 and clang6.0. I've also tested on MSVC to ensure I didn't break windows. It would be good to get a few more people to compile it and run the tests. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
v2-0001-Add-basic-support-for-using-the-POPCNT-and-SSE4.2.patch
Description: Binary data