Date: Thu, 31 Jan 2019 19:45:02 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Thomas Munro <thomas.munro@enterprisedb.com>
Subject: Re: Using POPCNT and other advanced bit manipulation instructions
Message-ID: <201901312245.y4phq2m6f3gy@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAKJS1f9WTAGG1tPeJnD18hiQW5gAk59fQ6WK-vfdAKEHyRg2RA@mail.gmail.com>
User-Agent: NeoMutt/20180716-1013-2c1e88
Precedence: bulk

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.
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:

  #ifdef HAVE__BUILTIN_CTZ
  int			(*pg_rightmost_one32) (uint32 word) = pg_rightmost_one32_choose;
  #else
  int			(*pg_rightmost_one32) (uint32 word) = pg_rightmost_one32_slow;
  #endif

  #ifdef HAVE__BUILTIN_CTZ
  /*
   * This gets called on the first call. It replaces the function pointer
   * so that subsequent calls are routed directly to the chosen implementation.
   */
  static int
  pg_rightmost_one32_choose(uint32 word)
  {
  ...

(You need declarations for the "choose" variants at the top of the file,
but that seems okay.)

Finally, the part in bitmapset.c is repetitive on the #ifdefs; I'd just
put at the top of the file something like 

  #if bms are 32 bits
  #define pg_rightmost_one(x) pg_rightmost_one32(x)
  #define pg_popcount(x) pg_popcount32(x)
  #elif they are 64 bits
  #define ...
  #else
  #error ...
  #endif

This way, each place that uses the functions does not need the ifdefs.

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.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services