Date: Mon, 4 Mar 2024 16:21:18 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: "Amonson, Paul D" <paul.d.amonson@intel.com>
Cc: Andres Freund <andres@anarazel.de>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	"Shankaran, Akash" <akash.shankaran@intel.com>,
	Noah Misch <noah@leadboat.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: Popcount optimization using AVX512
Message-ID: <20240304222118.GB3367383@nathanxps13>
References: <202402071013.reqe3bdnjjuj@alvherre.pgsql>
 <BL1PR11MB53042C623F17F17469B2C053DC4B2@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240209183450.snllkk7erharc73y@awork3.anarazel.de>
 <BL1PR11MB5304F65FD8D19B608F9E8624DC482@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240212203714.u4p7yeu7isuemmze@awork3.anarazel.de>
 <BL1PR11MB5304F0889AB4EB200AF44565DC572@BL1PR11MB5304.namprd11.prod.outlook.com>
 <BL1PR11MB5304C768A3002CD2C6EF672FDC5A2@BL1PR11MB5304.namprd11.prod.outlook.com>
 <BL1PR11MB53046A243932039BD545EBF4DC592@BL1PR11MB5304.namprd11.prod.outlook.com>
 <20240301214457.GA3024365@nathanxps13>
 <BL1PR11MB5304BFE26BAC25624508CFD2DC232@BL1PR11MB5304.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BL1PR11MB5304BFE26BAC25624508CFD2DC232@BL1PR11MB5304.namprd11.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/20240304222118.GB3367383%40nathanxps13>
Precedence: bulk

(Please don't top-post on the Postgres lists.)

On Mon, Mar 04, 2024 at 09:39:36PM +0000, Amonson, Paul D wrote:
> First, apologies on the patch. Find re-attached updated version.

Thanks for the new version of the patch.

>> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
>> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31)
>> +<< 31))
>>
>> IME this means that the autoconf you are using has been patched.  A
>> quick search on the mailing lists seems to indicate that it might be
>> specific to Debian [1].
>  
> I am not sure what the ask is here?  I made changes to the configure.ac
> and ran autoconf2.69 to get builds to succeed. Do you have a separate
> feedback here?

These LARGE_OFF_T changes are unrelated to the patch at hand and should be
removed.  This likely means that you are using a patched autoconf that is
making these extra changes.
 
> As for the refactoring, this was done to satisfy previous review feedback
> about applying the AVX512 CFLAGS to the entire pg_bitutils.c file. Mainly
> to avoid segfault due to the AVX512 flags. If its ok, I would prefer to
> make a single commit as the change is pretty small and straight forward.

Okay.  The only reason I suggest this is to ease review.  For example, if
there is some required refactoring that doesn't involve any functionality
changes, it can be advantageous to get that part reviewed and committed
first so that reviewers can better focus on the code for the new feature.
But, of course, that isn't necessary and/or isn't possible in all cases.

> I am not sure I understand the comment about the SIZE_VOID_P checks.
> Aren't they necessary to choose which functions to call based on 32 or 64
> bit architectures?

Yes.  My comment was that the patch appeared to make unnecessary changes to
this code.  Perhaps I am misunderstanding something here.

> Would this change qualify for Workflow A as described in [0] and can be
> picked up by a committer, given it has been reviewed by multiple
> committers so far? The scope of the change is pretty contained as well.

I think so.  I would still encourage you to create an entry for this so
that it is automatically tested via cfbot [0].

[0] http://commitfest.cputube.org/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com