public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Amonson, Paul D <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Shankaran, Akash <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Popcount optimization using AVX512
Date: Mon, 4 Mar 2024 16:21:18 -0600
Message-ID: <20240304222118.GB3367383@nathanxps13> (raw)
In-Reply-To: <BL1PR11MB5304BFE26BAC25624508CFD2DC232@BL1PR11MB5304.namprd11.prod.outlook.com>
References: <[email protected]>
<BL1PR11MB53042C623F17F17469B2C053DC4B2@BL1PR11MB5304.namprd11.prod.outlook.com>
<[email protected]>
<BL1PR11MB5304F65FD8D19B608F9E8624DC482@BL1PR11MB5304.namprd11.prod.outlook.com>
<[email protected]>
<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>
(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
view thread (26+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Popcount optimization using AVX512
In-Reply-To: <20240304222118.GB3367383@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox