test(linalg): cover the hamming kernel remainder loops per width - #8870
Open
LuciferYang wants to merge 1 commit into
Open
test(linalg): cover the hamming kernel remainder loops per width#8870LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
These focused tests make the existing kernel-tail contract explicit: the dispatched matrix covers every width-8 remainder, and the direct AVX2 matrix covers every width-4 remainder even when AVX-512 wins dispatch. The all-slot scalar oracle is proportionate for this test-only change.
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 this changes
Replaces
test_hamming_batch_u64with anrstestover 13 widths that asserts every output slot, and adds a direct test forhamming_batch_avx2's remainder loop. Net+82/-10, tests only.Why, stated narrowly
I started this believing no test ever executed the kernels' remainder loops. That was wrong, and I checked it rather than shipping the claim: the pairwise tests reach them, because a row sweep passes every width from
n - 1down to 1. Againstupstream/main, replacing the scalar remainder loop withwhile falsefails 3 existing tests, andchunks = n / 8ton.div_ceil(8)fails 5.So what this actually buys is two narrower things.
Per-width assertions. The old test used
n = 128and checked 4 of 128 slots against hardcoded values. A tail bug today surfaces as a mismatch somewhere among 499,500 pairs; this names the width and the slot.One real gap.
hamming_batch_simdreturns early on the AVX-512 branch whenavx512vpopcntdqandavx512fare both present, so on a VPOPCNTDQ host nothing reacheshamming_batch_avx2throughhamming_batch_u64. Its only other callers aretest_avx2_popcountatn = 8andtest_avx2_max_distanceatn = 4, both chunk-exact, so on such a host the AVX2 remainder loop is executed by no test at all.test_avx2_covers_tailcloses that.What the helper does
check_kernel_tailallocatesn + 8output slots, hands the kernel only&mut results[..n], checks every value against(query ^ target).count_ones(), and checks the 8 guard slots. 8 is the widest chunk store in the file, so one extra chunk iteration lands inside the allocation; two would still escape it, and the remainder loops cannot trip the guards at all since all three index through bounds-checkedresults[..]and would panic first. The guard is best-effort rather than guaranteed, because a store past the reborrowed slice is undefined behaviour the compiler may assume away.Query and target values are
0xDEAD_BEEF_CAFE_F00Dagainst golden-ratio multiples, rather than the oldquery = 0withtargets = 0..128. The old data set only the low 7 bits, so_mm256_shuffle_epi8saw two distinct nibble values and_mm256_sad_epu8summed a single nonzero byte lane.What I removed after review
An
test_avx512_covers_tailthat added nothing. Its feature gate is exactly the dispatch condition, so it only ran wherehamming_batch_u64already drives that kernel, over a wider set of widths through the same helper and oracle. Its one unique case,n = 6, moved into the dispatched rstest, which also closes a gap I had left: no case satisfiedn % 8 == 6, so the unrolled scalar fallback that runs on the ARM legs never finished with six leftover slots.Test plan
cargo test --profile ci -p lance-linalg --lib distance::hamming: 56 passed on aarch64, 65 onx86_64-apple-darwincargo clippy -p lance-linalg --all-targets -- -D warningsand the same for--target x86_64-apple-darwin: cleancargo fmt --all -- --check: cleann / 8ton.div_ceil(8)fails 1, 4, 9, 15Not covered: this machine is aarch64 and the x86_64 target runs under Rosetta, which reports no AVX2, so
test_avx2_covers_tailis compile-checked here and first executes on an x86 runner. Nothing in CI pinsavx512vpopcntdq, which is why the AVX-512 kernel gets no direct test rather than an unreliable one.