Hi John,
> You raised some interesting points, which deserve a thoughtful response. After
> sleeping on it, however I came to the conclusion that a sweeping change in
> runtime checks, with either of our approaches, has downsides and unresolved
> questions. Perhaps we can come back to it at a later time.
Sounds good to me. I did get derailed into something beyond the scope of this
patch. I will make a separate proposal once this is merged.
> Here's what I came up with in v11:
Some feedback on v11:
if ((exx[2] & (1 << 20)) != 0) /* SSE 4.2 */
{
pg_comp_crc32c = pg_comp_crc32c_sse42;
#ifdef USE_PCLMUL_WITH_RUNTIME_CHECK
if ((exx[2] & (1 << 1)) != 0) /* PCLMUL */
pg_comp_crc32c = pg_comp_crc32c_pclmul;
#endif
}
#ifdef USE_SSE42_CRC32C_WITH_RUNTIME_CHECK
else
pg_comp_crc32c = pg_comp_crc32c_sb8;
#endif
Is the #ifdef USE_SSE42_CRC32C_WITH_RUNTIME_CHECK at the right place? Shouldn’t
it guard SSE4.2 function pointer assignment?
/* WIP: configure checks */
#ifdef __x86_64__
#define USE_PCLMUL_WITH_RUNTIME_CHECK
#endif
Minor consideration: gcc 4.3 (released in 2011) is the only compiler that
supports -msse4.2 and not -mpclmul. gcc >= 4.4 supports both. If you are okay
causing a regression on gcc4.3, we could combine USE_PCLMUL_WITH_RUNTIME_CHECK
with USE_SSE42_CRC32C_WITH_RUNTIME_CHECK into a single macro to reduce the
number of #ifdef's in the codebase and simplify configure/meson compiler checks.
> 0001: same benchmark module as before
> 0002: For SSE4.2 builds, arrange so that constant input uses an inlined path
> so
> that the compiler can emit unrolled loops anywhere.
When building with meson, it looks like we build with -O2 and that is not
enough for the compiler to unroll the SSE42 CRC32C loop. It requires -O3 or -O2
with -funroll-loops (see https://gcc.godbolt.org/z/4Eaq981aT). Perhaps we
should check disassembly to see if the unroll is really happening on constant
input?
Also, the reason you have pg_comp_crc32c_sse42_inline defined separately in a
header file is because you want to (a) inline the function and (b) unroll for
constant inputs. Couldn't both of these be achieved by adding function
__attribute__((always_inline)) on the pg_comp_crc32c_sse42 function with the
added -funroll-loops compiler flag?
Raghuveer
