fix(linalg): use a lane-wise intrinsic for i32x8 multiplication on x86 - #8579
Open
LuciferYang wants to merge 2 commits into
Open
fix(linalg): use a lane-wise intrinsic for i32x8 multiplication on x86#8579LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
The guard test added with the `_mm256_mullo_epi32` fix was gated on `cfg(target_arch = "x86_64")`, so the aarch64 and loongarch64 arms of the same `Mul` impl had no coverage, and on a non-AVX2 x86 host the runtime feature check returned before any assertion ran. Un-gate the body, keep only the feature check arch-gated as `f32.rs` and `f64.rs` do, and add a negative and a wrapping case so the low-32-bits contract is pinned rather than just the all-positive squares. Also document what `Mul` promises (lane-wise, low 32 bits, wrapping) and record that the x86 arm's intrinsics carry no `#[target_feature]` gate.
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The intrinsic replacement fixes the x86 lane-shape bug at its source and matches the established lane-wise, low-32-bit behavior on the other architectures. The portable regression cases cover odd-lane loss, signed products, and wrapping overflow.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
impl Mul for i32x8's x86_64 arm called_mm256_mul_epi32(vpmuldq) where_mm256_mullo_epi32(vpmulld) was meant.vpmuldqmultiplies only the even 32-bit lanes and writes four 64-bit results, so four of the eight lanes were destroyed. Measured on an AVX2 host,[1, 2, ..., 8]squared:_mm256_mul_epi32[1, 0, 9, 0, 25, 0, 49, 0]_mm256_mullo_epi32[1, 4, 9, 16, 25, 36, 49, 64]Both intrinsics are AVX2, so the file's CPU requirement is unchanged. The aarch64 (
vmulq_s32) and loongarch64 (lasx_xvmul_w) arms were already lane-wise, and this brings x86 in line with them.The two intrinsics share a signature, so picking the wrong one is a silent wrong answer rather than a compile error, and nothing in the file wrote the contract down.
fn mulnow states it: lane-wise, low 32 bits, wrapping. The wrapping half is scoped tomuldeliberately, sincereduce_sumpanics on overflow in a debug build on x86_64 and loongarch64 while aarch64 wraps.Scope: latent, not a live wrong answer
Nothing in the repo multiplies
i32x8. The only use is as a gather index vector inf32x8::gather, which does no multiplication; the impl exists becauseSIMD's supertrait bound requiresMul. So no shipped result is wrong today. What made it worth fixing is that the file had an emptymod tests {}, so the next caller would have inherited the bug silently.Why the test is portable rather than x86-gated
The assertion is that multiplication is lane-wise, which is what all three arms promise, so gating the test on
cfg(target_arch = "x86_64")would cost more than it buys: on aarch64 the test would not exist,linux-armandmac-buildwould compile it out, and a copy-paste slip in the NEON arm (both halves readingself.0.0) would ship green. Only the feature check is arch-gated, followingf32.rsandf64.rs.That runtime
is_x86_feature_detected!("avx2")early return is load-bearing.qemu-pre-haswellrunscargo test --release -p lance-linalg --libunderqemu-x86_64 -cpu Nehalem, which has no AVX2, and these methods carry no#[target_feature]gate, so without the guard that job would SIGILL. Worth stating plainly, because it reads backwards: that job is the only one scoped tolance-linalg, and it is the one where this test asserts nothing. The assertions run onlinux-coverage-testandwindows-build.Three cases, because
[1..8]squared is narrow enough that a saturating or widening implementation would also pass it. Each was checked against both intrinsics on an AVX2 host, and mutation testing kills exactly one case per mutant, so none of them is padding:squaresmixed_signsvabsq_s32on both operandswraps_to_low_32_bitsvmull_s32+vqmovn_s64An all-zero case is deliberately absent:
vpmuldqreturns zeros too, so it discriminates nothing.One pre-existing precondition, now written down
The x86_64
struct i32x8gained a note that its intrinsics carry no#[target_feature]gate of their own, so callers must already be inside an AVX2-checked context. This is not a behavior change:Add,Sub,min,splatandloadare all in the same position, and it is currently unreachable because the one caller sits insidegather_avx2, which is#[target_feature(enable = "avx2")]behind a runtime check.Test plan
cargo test -p lance-linalg --libon aarch64-apple-darwin: 152 passedcargo test -p lance-linalg --lib --target x86_64-apple-darwin: 210 passedcargo clippy -p lance-linalg --all-targets -- -D warningson both targets: cleancargo fmt --all -- --checkandcargo doc -p lance-linalg --no-deps: cleanqemu-x86_64 -cpu maxto produce the table above; wrapping (65536² = 0) and sign (-3 × 7 = -21) confirmed there tooOut of scope
impl Mul for u8x16has the same family of defect with a different failure mode: x86 saturates (_mm_packus_epi16clamps to 255) while aarch64 and the portable arm wrap, sou8(15) * u8(31)is 255 on one and 209 on the other, and its test pins both behaviors behind#[cfg]instead of resolving them. Choosing one contract changes some platform's current output, so it belongs in its own change. Filed as #8578.