RE: Popcount optimization using AVX512

Hi,

I am not sure what "top-post" means but I am not doing anything different but using "reply to all" in Outlook. Please enlighten me. 😊

This is the new patch with the hand edit to remove the offending lines from the patch file. I did a basic test to make the patch would apply and build. It succeeded.

Thanks,
Paul

-----Original Message-----
From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
Sent: Monday, March 4, 2024 2:21 PM
To: Amonson, Paul D <paul(dot)d(dot)amonson(at)intel(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>; Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>; Shankaran, Akash <akash(dot)shankaran(at)intel(dot)com>; Noah Misch <noah(at)leadboat(dot)com>; Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>; Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>; pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Popcount optimization using AVX512

(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