-
Notifications
You must be signed in to change notification settings - Fork 21
Rename dist functions from SQ8 to SQ8-FP32 #888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request renames SQ8 distance functions and files to SQ8_FP32 to distinguish asymmetric distance computation (between SQ8 storage and FP32 query) from symmetric SQ8_SQ8 distance computation (between two SQ8 vectors).
Changes:
- Renamed distance functions from
SQ8_*toSQ8_FP32_*for InnerProduct, Cosine, and L2Sqr operations - Updated implementation selectors from
Choose_SQ8_*toChoose_SQ8_FP32_*across all SIMD architectures - Corrected parameter order in function signatures to reflect SQ8 storage as first parameter and FP32 query as second parameter
- Renamed benchmark files and test utilities to match new naming convention
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/tests_utils.h | Renamed helper functions and corrected parameter order for SQ8-FP32 distance calculations |
| tests/unit/test_spaces.cpp | Updated test names and function calls; contains critical bugs with SQ8_SQ8 test names incorrectly renamed |
| tests/benchmark/spaces_benchmarks/bm_spaces_sq8_fp32.cpp | Renamed benchmark class and updated function calls |
| tests/benchmark/benchmarks.sh | Updated benchmark script references from spaces_sq8 to spaces_sq8_fp32 |
| tests/benchmark/CMakeLists.txt | Updated DATA_TYPE list to use sq8_fp32 instead of sq8 |
| src/VecSim/spaces/spaces.cpp | Updated GetDistFunc to call renamed SQ8_FP32 functions |
| src/VecSim/spaces/functions/*.h | Renamed function declarations for all SIMD architectures (SVE, SVE2, SSE4, NEON, AVX2, AVX2_FMA, AVX512) |
| src/VecSim/spaces/functions/*.cpp | Updated function implementations and choosers across all SIMD architectures |
| src/VecSim/spaces/L2_space.h | Renamed L2_SQ8_GetDistFunc to L2_SQ8_FP32_GetDistFunc with updated documentation |
| src/VecSim/spaces/L2_space.cpp | Updated function implementation and all SIMD path selections |
| src/VecSim/spaces/L2/L2_*.h | Renamed L2 template functions and corrected parameter order/comments across all SIMD implementations |
| src/VecSim/spaces/L2/L2.h | Updated function signature and comments |
| src/VecSim/spaces/L2/L2.cpp | Renamed implementation function and corrected parameter order |
| src/VecSim/spaces/IP_space.h | Renamed IP and Cosine GetDistFunc to SQ8_FP32 variants with updated documentation |
| src/VecSim/spaces/IP_space.cpp | Updated function implementations and all SIMD path selections |
| src/VecSim/spaces/IP/IP_*.h | Renamed IP template functions and corrected parameter order/comments across all SIMD implementations |
| src/VecSim/spaces/IP/IP.h | Updated function signatures and comments |
| src/VecSim/spaces/IP/IP.cpp | Renamed implementation function and corrected parameter order |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/unit/test_spaces.cpp
Outdated
| /* ======================== Tests SQ8_SQ8 ========================= */ | ||
|
|
||
| TEST_F(SpacesTest, SQ8_SQ8_ip_no_optimization_func_test) { | ||
| TEST_F(SpacesTest, SQ8_SQ8_FP32_ip_no_optimization_func_test) { |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name is incorrectly updated. This test is for SQ8-to-SQ8 symmetric distance (both vectors are SQ8 quantized), not for SQ8-FP32 asymmetric distance. The test uses SQ8_SQ8 functions, so the name should remain "SQ8_SQ8_ip_no_optimization_func_test".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
he has a point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oopsi
tests/unit/test_spaces.cpp
Outdated
|
|
||
| // Test with constant vector (all same values) for L2 | ||
| TEST(SQ8_SQ8_EdgeCases, L2ConstantVectorTest) { | ||
| TEST(SQ8_SQ8_FP32_EdgeCases, L2ConstantVectorTest) { |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test suite name is incorrectly updated. It should remain "SQ8_SQ8_EdgeCases" as this test is for SQ8-to-SQ8 symmetric distance (both vectors are SQ8 quantized), not for SQ8-FP32 asymmetric distance. The test content uses SQ8_SQ8_L2Sqr which is a different function from SQ8_FP32_L2Sqr.
- Rename distance functions: SQ8_* -> SQ8_FP32_* for IP, Cosine, L2 - Update variable ordering to clarify pVect1=SQ8 storage, pVect2=FP32 query - Update test utilities and benchmark references - Rename Choose_SQ8_* -> Choose_SQ8_FP32_* implementation selectors
- Rename IP headers: IP_*_SQ8.h -> IP_*_SQ8_FP32.h - Rename L2 headers: L2_*_SQ8.h -> L2_*_SQ8_FP32.h - Rename benchmark file: bm_spaces_sq8.cpp -> bm_spaces_sq8_fp32.cpp
d048ec4 to
2bdb6f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #888 +/- ##
==========================================
- Coverage 97.11% 97.09% -0.03%
==========================================
Files 129 129
Lines 7493 7493
==========================================
- Hits 7277 7275 -2
- Misses 216 218 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
meiravgri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noice
| // Helper: compute Σ(q_i * y_i) for 8 elements (no dequantization) | ||
| static inline void InnerProductStepSQ8(const float *&pVect1, const uint8_t *&pVect2, | ||
| // pVect1 = SQ8 storage (quantized values), pVect2 = FP32 query | ||
| static inline void InnerProductStepSQ8(const uint8_t *&pVect1, const float *&pVect2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static inline void InnerProductStepSQ8(const uint8_t *&pVect1, const float *&pVect2, | |
| static inline void InnerProductStepSQ8_FP32(const uint8_t *&pVect1, const float *&pVect2, |
| __m512 v1 = _mm512_loadu_ps(pVec1); | ||
|
|
||
| // pVec1 = SQ8 storage (quantized values), pVec2 = FP32 query | ||
| static inline void SQ8_InnerProductStep(const uint8_t *&pVec1, const float *&pVec2, __m512 &sum) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static inline void SQ8_InnerProductStep(const uint8_t *&pVec1, const float *&pVec2, __m512 &sum) { | |
| static inline void SQ8_FP32_InnerProductStep(const uint8_t *&pVec1, const float *&pVec2, __m512 &sum) { |
| // Helper: compute Σ(q_i * y_i) for 4 elements (no dequantization) | ||
| static inline void InnerProductStepSQ8(const float *&pVect1, const uint8_t *&pVect2, | ||
| // pVect1 = SQ8 storage (quantized values), pVect2 = FP32 query | ||
| static inline void InnerProductStepSQ8(const uint8_t *&pVect1, const float *&pVect2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static inline void InnerProductStepSQ8(const uint8_t *&pVect1, const float *&pVect2, | |
| static inline void InnerProductStepSQ8_FP32(const uint8_t *&pVect1, const float *&pVect2, |
| // Load 4 float elements from query | ||
| __m128 v1 = _mm_loadu_ps(pVect1); | ||
| // pVect1 = SQ8 storage (quantized values), pVect2 = FP32 query | ||
| static inline void InnerProductStepSQ8(const uint8_t *&pVect1, const float *&pVect2, __m128 &sum) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static inline void InnerProductStepSQ8(const uint8_t *&pVect1, const float *&pVect2, __m128 &sum) { | |
| static inline void InnerProductStepSQ8_FP32(const uint8_t *&pVect1, const float *&pVect2, __m128 &sum) { |
| // Helper: compute Σ(q_i * y_i) for one SVE vector width (no dequantization) | ||
| static inline void InnerProductStepSQ8(const float *pVect1, const uint8_t *pVect2, size_t &offset, | ||
| // pVect1 = SQ8 storage (quantized values), pVect2 = FP32 query | ||
| static inline void InnerProductStepSQ8(const uint8_t *pVect1, const float *pVect2, size_t &offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| static inline void InnerProductStepSQ8(const uint8_t *pVect1, const float *pVect2, size_t &offset, | |
| static inline void InnerProductStepSQ8_FP32(const uint8_t *pVect1, const float *pVect2, size_t &offset, |
| dist_func_t<float> Choose_SQ8_FP32_IP_implementation_AVX512F(size_t dim); | ||
| dist_func_t<float> Choose_SQ8_FP32_Cosine_implementation_AVX512F(size_t dim); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does these exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, not sure why it was added
tests/unit/test_spaces.cpp
Outdated
| /* ======================== Tests SQ8_SQ8 ========================= */ | ||
|
|
||
| TEST_F(SpacesTest, SQ8_SQ8_ip_no_optimization_func_test) { | ||
| TEST_F(SpacesTest, SQ8_SQ8_FP32_ip_no_optimization_func_test) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
he has a point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Describe the changes in the pull request
Rename SQ8 distance functions and files to SQ8_FP32 to clarify that they compute asymmetric distance between SQ8 (quantized storage) and FP32 (query) vectors.
This distinguishes them from SQ8_SQ8 functions, which compute the symmetric distance between two SQ8 vectors.
Changes include:
Which issues this PR fixes
Main objects this PR modified
Mark if applicable