Re: What exactly is our CRC algorithm? - Mailing list pgsql-hackers

At 2015-02-10 14:30:51 +0200, hlinnakangas@vmware.com wrote:
>
> I propose that we add a new header file in src/port, let's call it
> crc_instructions.h […] the point of the crc_instructions.h header
> is to hide the differences between compilers and architectures.

Moving the assembly code/compiler intrinsics to a separate header is
fine with me, but I really don't like the proposed implementation at
all. Here are some comments:

1. I don't mind moving platform-specific tests for CRC32C instructions  to configure, but if we only define
PG_HAVE_CRC32C_INSTRUCTIONS,we  would anyway have to reproduce all that platform-specific stuff in  the header file. To
doit properly, I think we should generate the  right version of crc_instructions.h for the platform. Otherwise,  what's
thepoint? Might as well have only the complicated header.
 

2. At this point, I think we should stick with the _sse function rather  than a generic _hw one to "drive" the
platform-specificinstructions.  The structure of that function (n/8*(8bytes)+n%8*(bytes)) is specific  to what works
bestfor the SSE instructions. On another platform, we  may need something very similar, or very different.    With the
proposedstructure, _hw would inevitably become #ifdef'ed  non-trivially, e.g. to run a single-byte loop at the start to
align the main loop, but only for certain combinations of #defines.    I think we should consider what makes the most
sensewhen someone  actually wants to add support for another platform/compiler, and  not before.
 

3. I dislike everything about pg_crc32_instructions_runtime_check()—the  name, the separate
PG_CRC32C_INSTRUCTIONS_NEED_RUNTIME_CHECK#define,  the fact that you try to do the test "inline" rather than at
startup…
  Maybe you're trying to avoid checking separately at startup so that a  program that links with code from src/common
doesn'tneed to do its  own test. But I don't think the results are worth it.
 
  As for omitting the slicing-by-8 tables, if there's a more compelling  reason to do that than "Gentoo users might
likethat ;-)", I think it  should be done with a straightforward -DUSE_CRC_SSE style arrangement  rather than figuring
outthat you HAVE_CRC32C_INSTRUCTIONS but don't  NEED_RUNTIME_CHECK. If you want to build special-purpose binaries,
justpick whichever CRC implementation you like, done.
 

> I believe the CRC instructions are also available in 32-bit mode, so
> the check for __x86_64__ is not needed. Right?

I'm not sure, but I guess you're right.

> It would be nice to use GCC's builtin intrinsics,
> __builtin_ia32_crc32* where available, but I guess those are only
> available if you build with -msse4.2, and that would defeat the
> point of the runtime check.

Exactly.

-- Abhijit