On Tue, 13 Oct 2026 14:55:46 GMT, Vladimir Kozlov <[email protected]> wrote:
>> ### Description
>>
>> The CRC32 kernel used on machines without AVX-512
>> (MacroAssembler::kernel_crc32) folds 512 bits of input per iteration, 128
>> bits per each of four parallel streams, but it reduces the final 128-bit
>> state to 32 bits with the byte-wise table: two vpclmulqdq and a vpand fold
>> the state down to 64 bits using the K_M_64 constant, and the rest is folded
>> eight bits at a time with fold_8bit_crc32 - four folds on a 128-bit vector
>> register, then four more on a general purpose register.
>>
>> Each 8-bit fold computes crc = T[crc & 0xFF] ^ (crc >> 8) and depends on the
>> result of the previous fold, so the reduction is a serial chain of eight
>> table lookups whose addresses depend on the incoming data, plus five
>> XMM->GPR moves (four movdl to index the table, one to hand the value over to
>> the GPR folds).
>>
>> This change replaces the reduction with the folding and Barrett sequence
>> that kernel_crc32_avx512 already uses:
>> - vpclmulqdq folds the 128-bit state down to 96 and then to 64 bits using
>> the new K_128_64 constant;
>> - a Barrett reduction with the new K_64_32 constant cancels the upper 32
>> bits with two carry-less multiplies;
>> - pextrd extracts the CRC directly from the vector register; pextrd is
>> SSE4.1 only, so it is guarded with VM_Version::supports_sse4_1() and falls
>> back to vpsrldq + movdl;
>> - the byte-wise table is still used by the prologue and for the trailing
>> bytes, but not by the reduction anymore.
>>
>> K_128_64 and K_64_32 are added to _crc_by128_masks, following the naming and
>> one-bit pre-shift conventions of the existing entries so that the table
>> stays ordered by fold distance. K_M_64 was only used by the removed
>> reduction and is dropped; the AOT address table is updated since the
>> generated stub references the new entries. Only the non-AVX-512 CRC32 path
>> changes: kernel_crc32_avx512 and CRC32C are untouched.
>>
>> ### TEST
>> - env: Intel Xeon 6980P (Granite Rapids), single socket, 128 cores / 256
>> threads (SMT2), 3 NUMA nodes, 755 GiB RAM, Ubuntu 22.04.4 LTS, glibc 2.35
>> - JMH: test/micro/org/openjdk/bench/java/util/TestCRC32.java
>> - jvmArgs: "-Xms4g -Xmx4g -XX:UseAVX=2"
>> - results (ops/ms):
>> | size | baseline | patched | delta |
>> | ---: | ---: | ---: | ---: |
>> | 16 | 23588.77 | 23589.00 | +0.0% |
>> | 32 | 14586.33 | 19589.51 | +34.3% |
>> | 64 | 13845.50 | 18218.98 | +31.6% |
>> | 128 | 12564.96 | 16064.29 | +27.8% |
>> | 256 | 12075.03 | 15205.08 | +25.9% |
>> | 512 | 11147.73 | 13791.16 | +23.7% |
>> | 1024 | 9623.2...
>
> @LangruiZhou please enable GHA testing in your branch.
Hi @vnkozlov, thank you for the review comment. I understand the concern about
complexity.
> I am not sure about this optimization. On one hand, the performance
> improvement of the **micro** benchmark is impressive. On the other hand, your
> changes complicated the code which became harder to understand and maintain.
To clarify, the change does not introduce a new algorithm: it makes the
non-AVX-512 reduction follow the same L_128_done + L_barrett sequence already
used by kernel_crc32_avx512(), with the same K_128_64/K_64_32 constants and
masks. The adaptations for kernel_crc32() are mechanical only: the constants
come from _crc_by128_masks, and the masks are built in a register instead of
being loaded from the AVX-512 mask table.
Would you consider this acceptable, or would you prefer a different way to
structure/comment the reduction? I'll revise accordingly.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 8488:
>
>> 8486: pxor(xmm5, xmm5);
>> 8487: pcmpeqd(xmm5, xmm5);
>> 8488: pslldq(xmm5, 4);
>
> I don't understand what these 2 instructions do. I assume `pxor` clears
> `xmm5`.
`pxor` clears `xmm5` may be redundant. I'll remove `pxor` in the next patch.
The next 2 instructions build the 128-bit mask used by the following `pand`:
- `pcmpeqd xmm5, xmm5` sets every 32-bit lane to `0xffffffff`;
- `pslldq xmm5, 4` shifts the value left by four bytes, leaving
`0xffffffff_ffffffff_ffffffff_00000000` (low dword cleared, upper 96 bits set).
This is the same mask that `kernel_crc32_avx512` loads from
`_crc_by128_masks_avx512[1]`; the later `vpsrldq xmm5, 8` derives
`_crc_by128_masks_avx512[0]` `0x00000000_00000000_ffffffff_ffffffff` for the
second `pand`.
The corresponding code in `kernel_crc32_avx512` is:
// barrett reduction
bind(L_barrett);
// [masks + 1 * 16] = 0xffffffff_ffffffff_ffffffff_00000000:
// keep the high 96 bits.
vpand(xmm7, xmm7,
ExternalAddress(StubRoutines::x86::crc_by128_masks_avx512_addr() + 1 * 16),
Assembler::AVX_128bit, tmp2);
movdqu(xmm1, xmm7);
movdqu(xmm2, xmm7);
movdqu(xmm10, Address(table, 4 * 16));
pclmulqdq(xmm7, xmm10, 0x0);
pxor(xmm7, xmm2);
// [masks + 0] = 0x00000000_00000000_ffffffff_ffffffff:
// keep the low 64 bits.
vpand(xmm7, xmm7,
ExternalAddress(StubRoutines::x86::crc_by128_masks_avx512_addr()),
Assembler::AVX_128bit, tmp2);
movdqu(xmm2, xmm7);
pclmulqdq(xmm7, xmm10, 0x10);
pxor(xmm7, xmm2);
pxor(xmm7, xmm1);
pextrd(crc, xmm7, 2);
-------------
PR Comment: https://git.openjdk.org/jdk/pull/33342#issuecomment-6291109896
PR Review Comment: https://git.openjdk.org/jdk/pull/33342#discussion_r4325043331
