Re: [PATCH] SVE popcount support

On Thu, 28 Nov 2024 at 20:22, Malladi, Rama <rvmallad(at)amazon(dot)com> wrote:
>
> Attachments protected by Amazon: 0001-SVE-popcount-support.patch |
SVE-popcount-support-PostgreSQL.png |
> Amazon has replaced the attachments in this email with download links.
Downloads will be available until December 27, 2024, 15:43 (UTC+00:00).
Tell us what you think
> For more information click here
>
> Please find attached a patch to PostgreSQL implementing SVE popcount. I
used John Naylor's test_popcount module [0] to put together the attached
graphs. This test didn't show any regressions with a relatively small
number of bytes, and it showed the expected improvements with many bytes.
>
>
>
> [0]
https://postgr.es/m/CAFBsxsE7otwnfA36Ly44zZO+b7AEWHRFANxR1h1kxveEV=ghLQ@mail.gmail.com

Hi!
I did look inside this patch. This was implemented mostly in the same way
as the current instructure selecting code, which is good.

=== patch itself

1)
> // for small buffer sizes (<= 128-bytes), execute 1-byte SVE instructions
> // for larger buffer sizes (> 128-bytes), execute 1-byte + 8-byte SVE
instructions
> // loop unroll by 2
PostgreSQL uses /* */ comment style.

2)

> + if (bytes <= 128)
> + {
> + prologue_loop_bytes = bytes;
> + }
> + else
> + {
> + aligned_buf = (const char *) TYPEALIGN_DOWN(sizeof(uint64_t), buf) +
sizeof(uint64_t);
> + prologue_loop_bytes = aligned_buf - buf;
> + }

For a single line stmt PostgreSQL does not use parenthesis. Examples [0] &
[1]

[0]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=contrib/intarray/_int_bool.c;h=2b2c3f4029ec5cb887bdc6b01439b15271483bbf;hb=HEAD#l179
[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/pl/plpgsql/src/pl_handler.c;h=b18a3d0b97b111e55591df787143d015e7f1fdc5;hb=HEAD#l68

3) `if (bytes > 128)` Loop in pg_popcount_sve function should be commented.
There is too much code without any comment why it works. For example, If
original source of this is some paper or other work, we can reference it.

==== by-hand benching (I also use John Naylor's module)

non-patched

```
db1=# \timing
Timing is on.
db1=# select drive_popcount(10000000, 10000);
drive_popcount
----------------
64608
(1 row)

Time: 8886.493 ms (00:08.886) -- with small variance (+- 100ms)

db1=# select drive_popcount64(10000000, 10000);
drive_popcount64
------------------
64608
(1 row)

Time: 139501.555 ms (02:19.502) with small variance (+- 1-2sec)
```

patched

```
db1=# select drive_popcount(10000000, 10000);
drive_popcount
----------------
64608
(1 row)

Time: 8803.855 ms (00:08.804) -- with small variance
db1=# select drive_popcount64(10000000, 10000);
drive_popcount64
------------------
64608
(1 row)

Time: 200716.879 ms (02:21.717) -- with small variance
```

I'm not sure how to interpret these results. Looks like this does not help
much on a large $num?

--
Best regards,
Kirill Reshke