Re: What exactly is our CRC algorithm?

At 2015-03-25 19:18:51 +0200, hlinnaka(at)iki(dot)fi wrote:
>
> I think we'll need a version check there. […]
>
> You want to write that or should I?

I'm not familiar with MSVC at all, so it would be nice if you did it.

> How do you like this latest version of the patch otherwise?

I'm sorry, but I'm still not especially fond of it.

Apart from removing the startup check so that client programs can also
use the best available CRC32C implementation without jumping through
hoops, I don't feel that the other changes buy us very much.

Also, assuming that the point is that people who don't care about CRCs
deeply should nevertheless be able to produce special-purpose binaries
with only the optimal implementation included, we should probably have
some instructions about how to do that.

Thinking about what you were trying to do, I would find an arrangement
roughly like the following to be clearer to follow in terms of adding
new implementations and so on:

#if defined(USE_CRC_SSE42) || …can build SSE4.2 CRC code…
#define HAVE_CRC_SSE42 1
pg_crc32c_sse42() { … }
bool sse42_crc32c_available() { … }
pg_comp_crc32c = pg_crc32c_sse42;
#elif defined(USE_CRC_ARM) || …can build ARM CRC code…
#define HAVE_CRC_ARM 1
pg_crc32c_arm() { … }
bool arm_crc32c_available() { … }
pg_comp_crc32c = pg_crc32c_arm;
#endif

#define CRC_SELECTION 1
#if defined(USE_CRC_SSE42) || defined(USE_CRC_ARM) || …
#undef CRC_SELECTION
#endif

#ifdef CRC_SELECTION
pg_crc32c_sb8() { … }

pg_comp_crc32c_choose(…)
{
pg_comp_crc32c = pg_crc32c_sb8;

#ifdef HAVE_CRC_SSE42
if (sse42_crc32c_available())
pg_comp_crc32c = pg_crc32c_sse42;
#elif …
…
#endif

return pg_comp_crc32c(…);
}

pg_comp_crc32c = pg_crc32c_choose;
#endif

Then someone who wants to force the building of (only) the SSE4.2
implementation can build with -DUSE_CRC_SSE42. And if you turn on
USE_CRC_ARM when you can't build ARM code, it won't build. (The
HAVE_CRC_xxx #defines could also move to configure tests.)

If you don't specify any USE_CRC_xxx explicitly, then it'll build
whichever (probably) one it can, and select it at runtime if it's
available.

All that said, I do recognise that there are all relatively cosmetic
concerns, and I don't object seriously to any of it. On the contrary,
thanks for taking the time to review and work on the patch. Nobody
else has expressed an opinion, so I'll leave it to you to decide whether
to commit as-is, or if you want me to pursue the above approach instead.

In the realm of very minor nitpicking, here are a couple of points I
noticed in crc_instructions.h:

1. I think «if (pg_crc32_instructions_runtime_check())» would read
better if the function were named crc32c_instructions_available or
pg_crc32c_is_hw_accelerated, or something like that.

2. It documents pg_accumulate_crc32c_byte and
pg_accumulate_crc32c_uint64, but actually defines pg_asm_crc32b and
pg_asm_crc32q. If you update the code rather than the documentation,
_update_ may be slightly preferable to _accumulate_, and maybe the
suffixes should be _uint8 and _uint64.

3. The descriptions (e.g. "Add one byte to the current crc value.")
should also probably read "Update the current crc value for the given
byte/eight bytes of data".

Thanks.

-- Abhijit