Re: Optimize Arm64 crc32c implementation in Postgresql

On Thu, May 3, 2018 at 4:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
>>> Isn't there a hidden assumption about endianness there?

Right, thanks.

>> Yeah, I was wondering about that too, but does anyone actually run
>> ARMs big-endian?

I think all the distros I follow dropped big endian support in recent
years, but that's no excuse.

> After a bit more thought ... we could remove the magic constant,
> along with any assumptions about endianness, by doing it like this:
>
> result = (pg_comp_crc32c_armv8(0, &data, sizeof(data)) ==
> pg_comp_crc32c_sb8(0, &data, sizeof(data)));
>
> Of course we'd eat a few more cycles during the test, but it's hard
> to see that mattering.

I was thinking of proposing this:

- uint64 data = 42;
+ uint64 data = UINT64CONST(0x4242424242424242);

... and:

- result = (pg_comp_crc32c_armv8(0, &data, sizeof(data))
== 0xdd439b0d);
+ result = (pg_comp_crc32c_armv8(0, &data, sizeof(data))
== 0x54eb145b);

No strong preference though.

> It strikes me also that, at least for debugging purposes, it's seriously
> awful that you can't tell from outside what result this function got.
> Certainly the outcome that "pg_comp_crc32c_armv8 executed but returned
> the wrong answer" is not something that ought to go unremarked.
> We could elog the results, but I'm not very sure what log level is
> appropriate --- also, I doubt we want another log entry from every
> launched process, so how to prevent that?

I don't think *broken* CPUs are something we need to handle, are they?
Hmm, I suppose there might be a hypothetical operating system or ARM
chip that somehow doesn't raise SIGILL and returns garbage, and then
it's nice to fall back to the software version (unless you're 1/2^32
unlucky).

For debugging I was just putting a temporary elog in. Leaving one in
there at one of the DEBUG levels seems reasonable though.

--
Thomas Munro
http://www.enterprisedb.com