On Fri, 23 Oct 2026 22:37:20 GMT, Vladimir Kozlov <[email protected]> wrote:
>> `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); > > Add comments similar to kernel_crc32_avx512 code comments to show state of > mask. > > Looking on your AVX vs non-AVX code. It is unfortunate that you have to > duplicate exactly the same code but with different prefix encoding. I have > few comments. Thanks for the suggestions. I've added comments showing the mask state at construction and at both `pand` uses, plus the `64b fold` / `32b fold` stage comments similar to `kernel_crc32_avx512`. On the duplication, the 2-operand forms (`pclmulqdq`, `pxor`, `pand`, `psrldq`, `pslldq`, `movdqa`) are encoded as VEX when `UseAVX > 0` and as legacy SSE otherwise, so I merged the AVX and non-AVX sequences into one. The only remaining branch is the `pextrd` fallback for a non-AVX CPU without SSE4.1. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/33342#discussion_r4377473821
