Repository navigation
Request to revert: polyval: use avx+pclmulqdq features on x86/x86_64 #317 #346
Description
PCLMULQDQ: Introduced in 2010. Fairly Old. The code needs this. Safe to feature gate out.
VEX: vpclmulqdq in asm in this code base is VEX-encoded 128-bit (needs only AVX+PCLMULQDQ) AVX came out in 2011. Fairly Old.
Per Issue 184, the desire is to have vpclmulqdq asm produced. Per my benchmarking it is 0.5% faster, and potentially 4% faster depending on other factors.
Per issue 315, while AVX is fairly old, some... cheaper CPU's didn't/don't ship with it, breaking the goal of producing VEX (vpclmulqdq in asm).
So cool. Lets feature gate up this high. Fairly old. AVX2 came out in full in 2017 (AMD Zen1), so almost 10 years ago.
There's potential further discussion to have with AVX1 vs AVX2, but someone else can advocate for that.
VPCLMULQDQ the CPUID Flag: arrives with Intel Ice Lake and newer, and AMD Zen 4. Which rules out a lot of actually still in use CPU's. Feature gating this high prevents fast code on cpu's that support it. This is VEX.256. The code doesn't currently use such widths.
The performance hit is 0.26GiB/s vs 5.94GiB/s in my benchmarks on a LOT of genuinely still used CPU's.
; Legacy SSE (PCLMULQDQ flag)
pclmulqdq xmm0, xmm1, 0x00 ; xmm0 = xmm0 * xmm1
; VEX 128-bit (AVX + PCLMULQDQ — NOT the new flag)
vpclmulqdq xmm0, xmm1, xmm2, 0x00 ; 3-operand, 1 multiply
; VPCLMULQDQ flag — YMM (VEX.256)
vpclmulqdq ymm0, ymm1, ymm2, 0x11 ; 2 independent 64x64→128 multiplies
; VPCLMULQDQ + AVX512F — ZMM (EVEX.512)
vpclmulqdq zmm0, zmm1, zmm2, 0x10 ; 4 independent multiplies
Maybe in future, the codebase could get optimisations for these wider options. There's currently a separate issue for that.
#318 can still be technically correct. it IS optimised for VEX 128-bit which in asm is vpclmulqdq . I don't propose reverting it.
Steam hardware survey doesn't appear to expose these exact flags, but AVX 2 is at 95% and AVX512F is at 23%.
Activity
tarcieri commentedon Jul 10, 2026
Okay, so if I understand correctly: "vpclmulqdq" seems to mean two different things:
So I think the
#[target_feature(enable = "avx", enable = "pclmulqdq")]attributes in #317 are correct, and the real problem is:So we should just change that to:
...to match the
target_featuregating (and remove all the TODO comments aboutvpclmulqdq)Sound good?
(Ironically the goal of #317 was to make it work on more platforms, but it seems we accidentally made it work on fewer)
newpavlov commentedon Jul 10, 2026
Yeah, we should generally mirror what we use in
#[target_feature(...)]s. I think I expected thatvpclmulqdqis equivalent toavx + pclmulqdqbased on the reference, but I guess it's not true in practice.tarcieri commentedon Jul 10, 2026
Perhaps the
target_featureTODOs are fine. The real problem seems to be CPUID's idea ofvpclmulqdqis the AVX2 version.SirDMZ commentedon Jul 11, 2026
Per https://docs.rs/cpufeatures/latest/src/cpufeatures/x86.rs.html
("vpclmulqdq", "zmm", 1, ecx, 10)
So yes! their detect is filtering to what I read as AVX-512. Which is... worse than I had originally thought, but not that I can benchmark to detect. Oops. Unsure of why they might have done that.
We probably want, which to my understanding aligns with the code.
cpufeatures::new!(clmul, "avx", "pclmulqdq");
SirDMZ commentedon Jul 11, 2026
RustCrypto/utils@481df9d
Confirmed, they assumed vpclmulqdq is a AVX512 feature. So that's how their feature gating works.
tarcieri commentedon Jul 11, 2026
I opened #347 to address this.
@SirDMZ if you can confirm this fixes the issue, I can cut a release. Unfortunately I don't have any hardware old enough for this distinction to matter to test on myself.
SirDMZ commentedon Jul 11, 2026
TLDR: yes, this fixes it.
I forked polyval, and have been running that all day with this change applied.
I've updated the benchmark to reflect the discussed change. Test here is on Ryzen 3000 series.
results.txt
Edit: apparently github didn't like the formatting of the benchmark results.
newpavlov commentedon Jul 11, 2026
Hm, it may be worth to create a
cpufeaturesissue for that and look into whatstddoes with the feature.tarcieri commentedon Jul 11, 2026
This was released in
polyvalv0.7.2 in #348polyval: detect `avx`+`pclmulqdq` CPU features on x86 (RustCrypto#347)