fix: make the avx512vl_128 and avx512vl_256 arches build and pass - #1400
Open
DiamonDinoia wants to merge 3 commits into
Open
fix: make the avx512vl_128 and avx512vl_256 arches build and pass#1400DiamonDinoia wants to merge 3 commits into
DiamonDinoia wants to merge 3 commits into
Conversation
The avx512vl_128 overloads delegate to the common implementation with an unqualified call. That call is dependent, so it only resolves through ordinary lookup at the point of definition -- ADL cannot reach xsimd::kernel from arguments in namespace xsimd -- and neither name was declared before xsimd_avx512vl_128.hpp, which the ISA header includes ahead of xsimd_common.hpp. Building the test suite with -march=skylake-avx512 -mprefer-vector-width=128 therefore failed with 16 "no matching function for call to incr_if(..., xsimd::common)" errors. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
tobitset<N> packs N bool bytes into a bitmask by multiplying with a magic constant that gathers the N selected bits into the top N bits of the 8 * N bit product, so the result has to be shifted down by 8 * N - N. The N == 8 case shifts by 56 and is right; N == 4 shifted by 24 instead of 28 and N == 2 by 8 instead of 14, leaving the mask multiplied by 16 and 64 respectively. Exhaustively, the old shifts are wrong for 15 of the 16 inputs of the 4-bool block and 3 of the 4 inputs of the 2-bool block. Blocks smaller than 8 only occur for batch_bool of size 4 and 2, which only exist on avx512vl_128 and avx512vl_256. Neither compiled before the preceding commit, so the wrong code was unreachable. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
Both took the sign of the operand with an arithmetic shift and then reinterpreted the vector as a batch_bool. That construction assumes the boolean register is a vector, which is false for avx512vl_128 and avx512vl_256: they inherit the AVX kernels but carry k-register booleans, so the batch_bool constructor does not accept a __m256i and neither arch compiles. Comparing against zero produces the same mask with the right type on every architecture. It is also shorter for 64-bit lanes, where AVX2 has no vpsraq and the arithmetic shift is emulated -- per-function instruction counts at -O3 -mavx2: sadd<int64_t> 24 -> 14, ssub<int64_t> 21 -> 11, 32-bit and unsigned unchanged. Verified against a scalar reference over the type extremes and 64 pseudo-random values for all eight integer types on avx, avx2 and avx512: no mismatches before or after. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
DiamonDinoia
changed the title
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
1 participant
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.
Uh oh!
There was an error while loading. Please reload this page.