At 2015-02-10 14:30:51 +0200, hlinnakan...@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 do it properly, I think we should generate the right version of crc_instructions.h for the platform. Otherwise, what's the point? 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-specific instructions. The structure of that function (n/8*(8bytes)+n%8*(bytes)) is specific to what works best for the SSE instructions. On another platform, we may need something very similar, or very different. With the proposed structure, _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 sense when 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't need 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 like that ;-)", I think it should be done with a straightforward -DUSE_CRC_SSE style arrangement rather than figuring out that you HAVE_CRC32C_INSTRUCTIONS but don't NEED_RUNTIME_CHECK. If you want to build special-purpose binaries, just pick 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
