Re: Improve CRC32C performance on SSE4.2

On Mon, Mar 10, 2025 at 03:48:31PM +0700, John Naylor wrote:
> On Tue, Mar 4, 2025 at 2:11 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>> Overall, I wish we could avoid splitting things into separate files and
>> adding more header file gymnastics, but maybe there isn't much better we
>> can do without overhauling the CPU feature detection code.
>
> I wanted to make an attempt to make this aspect nicer. v13-0002
> incorporates deliberately compact and simple loops for inlined
> constant input into the dispatch function, and leaves the existing
> code alone. This avoids code churn and saves vertical space in the
> copied code. It needs a bit more commentary, but I hope this is a more
> digestible prerequisite to the CLMUL algorithm -- as a reminder, it'll
> be simpler if we can always assume non-constant input can go through a
> function pointer.

That is certainly more readable. FWIW I think it would be entirely
reasonable to replace the pg_crc32c_sse42.c implementation with a call to
this new pg_comp_crc32c_dispatch() function. Of course, you'd have to
split things up like:

pg_attribute_no_sanitize_alignment()
static inline
pg_crc32c
pg_comp_crc32c_sse42_inline(pgcrc32c crc, const void *data, size_t len)
{
const unsigned char *p = data;

#ifdef __x86_64__
for (; len >= 8; p += 8, len -= 8)
crc = _mm_crc32_u64(crc, *(const uint64 *) p);
#endif
for (; len >= 4; p += 4, len -= 4)
crc = _mm_crc32_u32(crc, *(const uint32 *) p);
for (; len > 0; len--)
crc = _mm_crc32_u8(crc, *p++)
return crc;
}

pg_attribute_no_sanitize_alignment()
static inline
pg_crc32c
pg_comp_crc32c_dispatch(pg_crc32c crc, const void *data, size_t len)
{
if (__builtin_constant_p(len))
return pg_comp_crc32c_sse42_inline(crc, data, len);
return pg_comp_crc32c_sse42(crc, data, len);
}

And then in pg_crc32c_sse42.c:

pg_attribute_no_sanitize_alignment()
pg_attribute_target("sse4.2")
pg_crc32c
pg_comp_crc32c_sse42(pg_crc32c crc, const void *data, size_t len)
{
return pg_comp_crc32c_sse42_inline(crc, data, len);
}

Granted, that adds back some of the complexity you were trying to avoid
(and is very similar to your v12 patches), but it's pretty compact and
avoids some code duplication. FTR I don't feel too strongly about this,
but on balance I guess I'd be okay with a tad more complexity than your
v13 patches if it served a useful purpose.

--
nathan