At 2014-12-30 16:05:50 +0200, hlinnakangas@vmware.com wrote:
>
> A couple of quick comments:
>
> bswap32 is unused on on little-endian systems. That will give a
> compiler warning.
Huh. I don't get a warning, even when I add -Wunused to the build flags.
But since you mention it, it would be better to write the function thus:
static inline uint32 cpu_to_le32(uint32 x) { #ifndef WORDS_BIGENDIAN return x; #elif defined(__GNUC__)
||defined(__clang__) return __builtin_bswap32(x); #else return ((x << 24) & 0xff000000) |
((x<< 8) & 0x00ff0000) | ((x >> 8) & 0x0000ff00) | ((x >> 24) & 0x000000ff); #endif
}
> pg_comp_crc32c_sse […] fetches the 8-byte chunks from only 4-byte
> aligned addresses. Is that intentional?
Thanks for spotting that. I had meant to change the test to "& 7". But
again, now that you mention it, I'm not sure it's necessary. The CRC32*
instructions don't have the usual SSE alignment requirements, and I see
that the Linux kernel (among other implementations) process eight bytes
at a time from the start of the buffer and then process the remaining
bytes one at a time. I'll do a bit more research and post an update.
Thanks for having a look.
-- Abhijit
