Implement AVX-512 IFMA vpmadd52 intrinsics - #5250
Conversation
88e7296 to
3d8aed6
Compare
I'm afraid I don't understand these words. ;) |
|
Cc @newpavlov @apoelstra @Kixunil for some crypto expertise |
There's nothing crypto-specific about the instruction itself - it's a general primitive for extended-precision integer arithmetic (emulating wider-than-64-bit integers across SIMD lanes). It shows up in bignum and modular-arithmetic code, which is common in crypto/ZK, but the instruction itself is just integer math. Miri's AVX-512 coverage is nearly complete, this is a small, semantically-simple two-instruction gap closure in it |
What do you mean by that? I thought AVX-512 has thousands of intrinsics and we support a tiny fraction... |
|
|
Well the entire point is that there's too many of them, we don't have the capacity to review and maintain implementations for all of them. Hence my questions for why we should invest into having the ones you are suggesting here. :) |
Our use case is verifying an accelerated cryptography library under Miri. Its optimized path uses +avx512ifma, and Miri currently stops when it reaches these intrinsics. Supporting them lets us run Miri over the actual accelerated implementation instead of disabling that path during verification or pinning the fork. |
|
There's no actual cryptography in this PR -- it just vpmadd52uq function, which does look right to me (in that it's shaped like a multiplication function with some simd projection/lifting surrounding it). Though I can certainly believe that an AVX-accelerated hashing function is blocked on it being present. So I did a cursory review but I think actually Miri and/or AVX/SIMD expertise would be more useful. |
There was a problem hiding this comment.
Thanks! The implementation does seem to match the comments. I have a hard time making more sense of it though since it seems like a completely arbitrary operation. 52 and 104 bits are very strange numbers.^^
@folkertdev I'd appreciate if you could take a look.
| let [z, x, y] = this.check_shim_sig_unadjusted(link_name, args)?; | ||
|
|
||
| let high = unprefixed_name.starts_with("vpmadd52h"); | ||
| vpmadd52uq(this, z, x, y, high, dest)?; |
There was a problem hiding this comment.
There's no reason to make this a function if it is only called in one place.
| if is_x86_feature_detected!("avx512ifma") { | ||
| test_avx512ifma(); | ||
| } else { | ||
| // Older AVX-512 silicon (e.g. Skylake-SP) lacks IFMA. |
There was a problem hiding this comment.
| // Older AVX-512 silicon (e.g. Skylake-SP) lacks IFMA. | |
| // Older AVX-512 CPUs (e.g. Skylake-SP) lack IFMA. |
| } | ||
|
|
||
| #[target_feature(enable = "avx512ifma,avx512vl")] | ||
| unsafe fn test_avx512ifma() { |
There was a problem hiding this comment.
Does stdarch have tests we could copy here?
Did you confirm that these tests are correct by running this on real hardware? I don't have access to AVX512 hardware so I cannot validate this.
There was a problem hiding this comment.
There was a problem hiding this comment.
and I have avx512 hardware now, the tests run and succeed natively.
|
|
||
| /// Multiply the low unsigned 52-bit integers in each 64-bit lane of `x` and | ||
| /// `y`, producing a 104-bit product, then add either its low (`high == | ||
| /// false`) or high (`high == true`) 52-bit half to the full 64-bit lane of |
There was a problem hiding this comment.
What happens with the remaining 12 bits in each lane of x and y? The implementation apparently ignores them. Is that correct? The comment should make that explicit.
Also, the "then add z part" is also per-lane, right? The comment does not make that clear.
There was a problem hiding this comment.
It might be better to follow the naming convention from the intel docs? or at least the ordering.
But yes as far as I can tell the remaining bits are ignored completely, and the "z part" is per-lane.
| /// Multiply the low unsigned 52-bit integers in each 64-bit lane of `x` and | ||
| /// `y`, producing a 104-bit product, then add either its low (`high == | ||
| /// false`) or high (`high == true`) 52-bit half to the full 64-bit lane of | ||
| /// `z` with wrapping arithmetic. |
There was a problem hiding this comment.
Is the "wrapping" case covered by the test?
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
5 participants
Adds shims for
llvm.x86.avx512.vpmadd52l.uq.{128,256,512}andvpmadd52h.uq.{128,256,512}, used by_mm512_madd52lo_epu64/_mm512_madd52hi_epu64and their 128/256-bit variants.Semantics per the Intel intrinsics guide: multiply the low 52 bits of each 64-bit lane of the two multiplicands into a 104-bit product, then add the selected 52-bit half to the full 64-bit accumulator lane with wrapping arithmetic. Feature gates follow the existing avx512 shims:
avx512ifma, plusavx512vlfor the narrower widths.Tests cover the
(2^52 - 1)^2saturation pattern for both halves, accumulator wraparound, and the low-52-bit masking of the multiplicands, at all three widths. Motivation: interpreting radix-52 big-integer cryptography (the vpmadd52-based Montgomery multiplication pattern) under Miri.