Skip to content

Conversation

@dor-forer
Copy link
Collaborator

@dor-forer dor-forer commented Jan 7, 2026

Describe the changes in the pull request

Fix and datpat the Fp32 SQ8 functions and tests to fit the new stracutre of the query vector.
[elements][sum]

Which issues this PR fixes

  1. MOD-13392

Main objects this PR modified

  1. ...
  2. ...

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

- Implemented inner product and cosine distance functions for SQ8-to-SQ8 vectors in SVE, NEON, and AVX512 architectures.
- Added corresponding distance function selection logic in IP_space.cpp and function headers in IP_space.h.
- Created benchmarks for SQ8-to-SQ8 distance functions to evaluate performance across different architectures.
- Developed unit tests to validate the correctness of the new distance functions against expected results.
- Ensured compatibility with existing optimization features for various CPU architectures.
- Implemented NEON, SVE, and AVX512F optimized functions for calculating L2 squared distance between SQ8 (scalar quantized 8-bit) vectors.
- Introduced helper functions for processing vector elements using NEON and SVE intrinsics.
- Updated L2_space.cpp and L2_space.h to include new distance function for SQ8-to-SQ8.
- Enhanced AVX512F, NEON, and SVE function selectors to choose the appropriate implementation based on CPU features.
- Added unit tests to validate the correctness of the new L2 squared distance functions.
- Updated benchmark tests to include performance measurements for the new implementations.
… using AVX512 VNNI; add benchmarks and tests for new functionality
…VE, and AVX512; add corresponding selection functions and update tests for consistency.
…update benchmarks and tests for new functionality
- Updated distance function declarations in IP_space.h to clarify that SQ8-to-SQ8 functions use precomputed sum/norm.
- Removed precomputed distance function implementations for AVX512F, NEON, and SVE architectures from their respective source files.
- Adjusted benchmark tests to remove references to precomputed distance functions and ensure they utilize the updated quantization methods.
- Modified utility functions to support the creation of SQ8 quantized vectors with precomputed sum and norm.
- Updated unit tests to reflect changes in the quantization process and removed tests specifically for precomputed distance functions.
…nsistency

- Updated include paths in AVX512F_BW_VL_VNNI.cpp to reflect new naming conventions.
- Modified unit tests in test_spaces.cpp to streamline vector initialization and quantization processes.
- Replaced repetitive code with utility functions for populating and quantizing vectors.
- Enhanced assertions in tests to ensure optimized distance functions are correctly chosen and validated.
- Removed unnecessary parameters from utility functions to simplify their interfaces.
- Improved test coverage for edge cases, including zero and constant vectors, ensuring accuracy across various scenarios.
- Refactor the SQ8 inner product computation to eliminate unnecessary dequantization steps, improving performance.
- Introduce a new helper function `InnerProductStepSQ8` that computes the inner product directly using quantized values.
- Update the main inner product function `SQ8_InnerProductSIMD_SVE_IMP` to utilize the new helper function, streamlining the computation process.
- Modify the test suite to validate the new implementation, ensuring correctness against the baseline non-optimized version.
- Add edge case tests for self-distance, symmetry, zero vectors, constant vectors, and extreme values to ensure robustness of the SQ8 cosine distance function.
- Introduce utility functions for preprocessing and populating SQ8 queries, enhancing test clarity and maintainability.
…to dorer-fp32-sq8-dist-functions-ip-cosine
…to dorer-fp32-sq8-dist-functions-ip-cosine
…to dorer-fp32-sq8-dist-functions-ip-cosine
@dor-forer dor-forer requested a review from meiravgri January 7, 2026 17:20
@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.13%. Comparing base (f6df960) to head (77d03be).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #882   +/-   ##
=======================================
  Coverage   97.13%   97.13%           
=======================================
  Files         129      129           
  Lines        7614     7615    +1     
=======================================
+ Hits         7396     7397    +1     
  Misses        218      218           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@meiravgri meiravgri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tight

#include <arm_sve.h>
#include <iostream>
#include <string.h>
using sq8 = vecsim_types::sq8;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using sq8 = vecsim_types::sq8;
using sq8 = vecsim_types::sq8;

TEST(SQ8_EdgeCases, CosineConstantVectorTest) {
auto optimization = getCpuOptimizationFeatures();
size_t dim = 128;
std::vector<float> v_const(dim + 1, 0.5f);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isnt the interesting case is when the quantized vector has constant values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right

Comment on lines +329 to 330
test_utils::SQ8_NotOptimized_InnerProduct(v1_orig.data(), v2_compressed.data(), dim);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont we want to test naive to SQ8_NotOptimized_InnerProduct with differnt vectors like we did for SQ8_SQ8?

SQ8_InnerProduct((const void *)v1_orig.data(), (const void *)v2_compressed.data(), dim);

ASSERT_NEAR(dist, baseline, 0.01) << "SQ8_InnerProduct failed to match expected distance";
// Since we're comparing identical vectors, the inner product distance should be close to 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see that all other types also validate we get 0 distance for idnetical vectors
consider keeping this test here as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one:
TEST(SQ8_EdgeCases, SelfDistanceCosine)


dist_func_t<float> arch_opt_func;
float baseline = SQ8_Cosine(v1_orig.data(), v2_quantized.data(), dim);
float baseline = test_utils::SQ8_NotOptimized_Cosine(v1_orig.data(), v2_quantized.data(), dim);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually compare to the naive calculation no?

@dor-forer dor-forer requested a review from meiravgri January 8, 2026 15:47
@dor-forer dor-forer enabled auto-merge January 8, 2026 15:56
@dor-forer dor-forer added this pull request to the merge queue Jan 8, 2026
Merged via the queue into main with commit bdcbf80 Jan 8, 2026
18 checks passed
@dor-forer dor-forer deleted the dorer-fp32-sq8-dist-functions-ip-cosine branch January 8, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants