Enable SVE Support for L2 Metric Computation in FP32 #969

adarshs1310 · 2024-11-29T10:34:36Z

Description:
This PR introduces SVE (Scalable Vector Extension) enablement for L2 metric computation in FP32. The changes enhance performance for most indexing methods compared to NEON, with observed speed-ups across multiple algorithms.

Changes in This PR:

Added SVE optimizations for L2 metric computation in FP32.
Updated CMakeLists.txt to include support for -march=armv8-a+sve.
Refactored compute kernels to leverage SVE intrinsics.

Benchmark Results:

Performance benchmarks(32 vcpus) were conducted using both NEON and SVE on ARM architecture. Below are the results showcasing execution times (in seconds):

Key observations:

All of the algorithms exhibit performance gains with SVE when compared with NEON.

Note: SVE support has been made optional, as not all functions have been fully enabled yet. To utilize SVE, please compile with -march=armv8-a+sve.

/kind feature
Fixes #782

sre-ci-robot · 2024-11-29T10:34:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adarshs1310
To complete the pull request process, please assign zhengbuqian after the PR has been reviewed.
You can assign the PR to them by writing /assign @zhengbuqian in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sre-ci-robot · 2024-11-29T10:34:46Z

Welcome @adarshs1310! It looks like this is your first PR to zilliztech/knowhere 🎉

mergify · 2024-11-29T10:35:13Z

@adarshs1310 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

alexanderguzhva · 2024-11-29T22:00:37Z

src/simd/distances_sve.cc

+    svbool_t pg = svptrue_b32();
+
+    while (i < d) {
+        if (d - i < svcntw())


I believe that this if condition is not needed, just pg = svwhilelt_b32(i, d); should be sufficient

Thank you so much, @alexanderguzhva , for the valuable suggestions. During development, we considered this approach as well, and our reason for going with the current approach is as follows:

Using the if condition to update pg only in the last iteration avoids unnecessary updates and reduces the dependency chain introduced by the svwhilelt instruction. This optimization minimizes stalls caused by these dependencies, allowing the processor pipeline to operate more efficiently.

alexanderguzhva · 2024-11-29T22:09:07Z

cmake/libs/libfaiss.cmake

@@ -48,7 +48,7 @@ endif()

 if(__AARCH64)
  set(UTILS_SRC src/simd/hook.cc src/simd/distances_ref.cc
-                src/simd/distances_neon.cc)
+                src/simd/distances_neon.cc src/simd/distances_sve.cc)


I believe that this is not sufficient.
Knowhere is designed as a library that picks function pointers according to CPU capabilities, detected upon the start.
For example, SSE / AVX2 / AVX512 code files have different corresponding compile options

knowhere/cmake/libs/libfaiss.cmake

Lines 37 to 40 in 1cb9937

target_compile_options(utils_sse PRIVATE -msse4.2 -mpopcnt)

target_compile_options(utils_avx PRIVATE -mfma -mf16c -mavx2 -mpopcnt)

target_compile_options(utils_avx512 PRIVATE -mfma -mf16c -mavx512f -mavx512dq

-mavx512bw -mpopcnt -mavx512vl)

So, it seems to be logical that the new SVE code should also contain some form of different flags, such as -march=armv8-a+sve for distances_sve.cc. And I don't believe that I see this.

Thank you for your valuable feedback @alexanderguzhva

We considered two approaches: first, to keep Neon as the default until all FP32 functions for SVE have been fully implemented. Based on this, we decided to proceed with this approach.

Your approach is absolutely correct, and we agree that the Neon fallback for non-SVE functions can be effectively handled using hook.cc.

We have incorporated these changes into our solution.

adarshs1310 · 2024-11-30T07:42:23Z

@alexanderguzhva @foxspy @hhy3

Requesting updates to the CI pipeline to address GCC header file conflicts.

Since Ubuntu 22.04 defaults to GCC-11, there is a risk of incorrect header usage when GCC-12 is installed. To resolve this, the GCC-11 header files should be removed after installing GCC-12.

The necessary changes have been incorporated into the ARM-based Ubuntu 22.04 Docker configuration as part of this PR, ensuring proper SVE compatibility. Kindly review and consider these adjustments for consistent and accurate builds. Thank you!

adarshs1310 · 2024-11-30T10:49:44Z

/kind feature

codecov · 2024-12-02T16:22:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.87%. Comparing base (3c46f4c) to head (09925c7).
Report is 269 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##           main     #969       +/-   ##
=========================================
+ Coverage      0   73.87%   +73.87%     
=========================================
  Files         0       82       +82     
  Lines         0     6916     +6916     
=========================================
+ Hits          0     5109     +5109     
- Misses        0     1807     +1807

see 82 files with indirect coverage changes

alexanderguzhva · 2024-12-03T15:07:50Z

@adarshs1310 would you please rebase on top of master? it contains a fix for UT. Thanks.

adarshs1310 · 2024-12-03T15:24:18Z

Sure @alexanderguzhva! The rebase has been done now!

adarshs1310 · 2024-12-09T07:08:44Z

Hi @alexanderguzhva , @foxspy , @hhy3 ,

Could you please look into the SSE fix when you have a chance? The ARM CI issue has already been handled in our code.

Thank you so much for your support!

foxspy · 2024-12-09T15:50:06Z

Hi @alexanderguzhva , @foxspy , @hhy3 ,

Could you please look into the SSE fix when you have a chance? The ARM CI issue has already been handled in our code.

Thank you so much for your support!

SSE's CI will not block; but ARM's CI fails

alexanderguzhva · 2024-12-09T17:36:30Z

@adarshs1310 I was able to compile and run knowhere on AWS Graviton 3 using GCC-12. I see the following error during unit tests, which seems to be a minor precision issue. Still, would you be able to find the root of this problem?

-------------------------------------------------------------------------------
Test Brute Force with input ids
-------------------------------------------------------------------------------
/home/ubuntu/zilliz/knowhere_sve/knowhere/tests/ut/test_bruteforce.cc:201
...............................................................................

/home/ubuntu/zilliz/knowhere_sve/knowhere/tests/ut/test_bruteforce.cc:243: FAILED:
  REQUIRE( gt_dis[i] == dis[i] )
with expansion:
  156769.3125f == 156769.32812f

Other unit tests pass.

The compilation is the following (given that you have created a profile for GCC 12 for conan):

mkdir build
cd build
conan install .. --build=missing -o with_diskann=True -o with_ut=True -o with_benchmark=True -s compiler.libcxx=libstdc++11 -c tools.build:cxxflags+=[\"-mcpu=neoverse-512tvb\",\"-march=native\"] -s build_type=Release --profile=gcc12
conan build ..

adarshs1310 · 2024-12-11T03:15:54Z

@adarshs1310 the solution of switching to -O2 is totally unacceptable, because it is compiler-dependent and reduces the performance. Please modify the unit tests to accept a higher threshold for a relative error.

Sure! @alexanderguzhva I understand your point, I have reverted back to O3 and made necessary changes in the test case. The branch has been rebased to head of current main as well.

ci/docker/builder/cpu/ubuntu22.04/arm64/Dockerfile

Presburger · 2024-12-13T07:56:39Z

CMakeLists.txt

+    endif()
+
+endif()
+


delete those lines, compiler know where to find arm_neon.h

have tried, can build pass without this file change

@Presburger ,

Initially, my plan was to keep SVE support optional until all the functions were implemented. For this reason, I provided the -march=armv8-a+sve flag as an optional tag for users to add. However, as per the discussion with @alexanderguzhva , we have now hardcoded -march=armv8-a+sve in the faiss CMake file. Consequently, I agree that we can remove the optional flag from CMake.

This approach works well after removing the optional tag. However, I had intentionally added those lines earlier because I believed the CI setup was incomplete. The reason for this was that not all users rely directly on the Docker environment provided by the CI. Some build the project manually, and if their machine has a different GCC version and they haven’t properly updated their alternatives, it could have caused issues but atleast they will get to know which vection of gcc was used for the header.

While the CI has now been fixed, future developers may still encounter problems in similar situations as and when they build it on local machine. We can consider a better long-term solution to address this later on.

adarshs1310 · 2024-12-16T04:49:21Z

@Presburger Hey, I noticed the do-not-merge/hold label was added 3 days ago. Could you let me know the reason for the hold and if there’s anything I can address to help move this forward?

@alexanderguzhva @foxspy @cydrain

Is this something at my end? because the UT were passing previously with same code base.

adarshs1310 · 2024-12-17T05:22:02Z

Rebase has been done

@foxspy @alexanderguzhva @hhy3

alexanderguzhva · 2024-12-17T15:29:59Z

src/simd/distances_sve.cc

+#include <cmath>
+
+#include "faiss/impl/platform_macros.h"
+#pragma GCC optimize("O3,fast-math,inline")


@adarshs1310 Could you please just remove this hacky pragma?
Thanks.

Sure @alexanderguzhva I have removed it. Thanks!

Signed-off-by:Adarsh Srivastava <adarsh.srivastava@fujitsu.com> Signed-off-by: Adarsh Srivastava <Adarsh.Srivastava@fujitsu.com>

alexanderguzhva · 2024-12-18T20:34:22Z

lgtm

sre-ci-robot requested review from foxspy and hhy3 November 29, 2024 10:34

sre-ci-robot added the size/L label Nov 29, 2024

mergify bot added the needs-dco label Nov 29, 2024

mergify bot added the do-not-merge/missing-related-issue label Nov 29, 2024

adarshs1310 force-pushed the sve-l2-fp32 branch 2 times, most recently from 1783ca6 to 4186e1a Compare November 29, 2024 11:31

mergify bot added dco-passed and removed needs-dco labels Nov 29, 2024

adarshs1310 force-pushed the sve-l2-fp32 branch 2 times, most recently from e304e26 to fe28988 Compare November 29, 2024 12:29

alexanderguzhva reviewed Nov 29, 2024

View reviewed changes

adarshs1310 force-pushed the sve-l2-fp32 branch 2 times, most recently from 97b3ee0 to dde2c81 Compare November 30, 2024 07:12

sre-ci-robot added the kind/feature label Nov 30, 2024

mergify bot removed the do-not-merge/missing-related-issue label Nov 30, 2024

adarshs1310 force-pushed the sve-l2-fp32 branch from dde2c81 to 9276ee8 Compare December 3, 2024 05:04

adarshs1310 requested a review from alexanderguzhva December 3, 2024 13:07

adarshs1310 force-pushed the sve-l2-fp32 branch from 9276ee8 to a626265 Compare December 3, 2024 15:21

cydrain reviewed Dec 12, 2024

View reviewed changes

ci/docker/builder/cpu/ubuntu22.04/arm64/Dockerfile Outdated Show resolved Hide resolved

adarshs1310 force-pushed the sve-l2-fp32 branch 3 times, most recently from 2454f3d to 94333f4 Compare December 13, 2024 02:27

Presburger reviewed Dec 13, 2024

View reviewed changes

mergify bot added needs-dco and removed dco-passed labels Dec 13, 2024

adarshs1310 force-pushed the sve-l2-fp32 branch from 579263d to eb43b9e Compare December 13, 2024 08:27

mergify bot added dco-passed and removed needs-dco labels Dec 13, 2024

Presburger added the do-not-merge/hold label Dec 13, 2024

mergify bot added the ci-passed label Dec 16, 2024

adarshs1310 force-pushed the sve-l2-fp32 branch from eb43b9e to 704321d Compare December 17, 2024 04:26

mergify bot added needs-dco and removed dco-passed ci-passed labels Dec 17, 2024

adarshs1310 force-pushed the sve-l2-fp32 branch from 704321d to e6abc41 Compare December 17, 2024 04:28

mergify bot added dco-passed and removed needs-dco labels Dec 17, 2024

alexanderguzhva reviewed Dec 17, 2024

View reviewed changes

adarshs1310 force-pushed the sve-l2-fp32 branch from e6abc41 to fba1adb Compare December 17, 2024 15:35

Enable SVE Support for L2 Metric Computation in FP32

09925c7

Signed-off-by:Adarsh Srivastava <adarsh.srivastava@fujitsu.com> Signed-off-by: Adarsh Srivastava <Adarsh.Srivastava@fujitsu.com>

adarshs1310 force-pushed the sve-l2-fp32 branch from fba1adb to 09925c7 Compare December 17, 2024 15:39

mergify bot added the ci-passed label Dec 17, 2024

adarshs1310 requested review from alexanderguzhva and Presburger December 18, 2024 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable SVE Support for L2 Metric Computation in FP32 #969

Enable SVE Support for L2 Metric Computation in FP32 #969

adarshs1310 commented Nov 29, 2024 •

edited

Loading

sre-ci-robot commented Nov 29, 2024

sre-ci-robot commented Nov 29, 2024

mergify bot commented Nov 29, 2024

alexanderguzhva Nov 29, 2024

adarshs1310 Nov 30, 2024

alexanderguzhva Nov 29, 2024

adarshs1310 Nov 30, 2024

adarshs1310 commented Nov 30, 2024

adarshs1310 commented Nov 30, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

alexanderguzhva commented Dec 3, 2024

adarshs1310 commented Dec 3, 2024

adarshs1310 commented Dec 9, 2024

foxspy commented Dec 9, 2024

alexanderguzhva commented Dec 9, 2024 •

edited

Loading

adarshs1310 commented Dec 11, 2024

Presburger Dec 13, 2024

cydrain Dec 13, 2024

adarshs1310 Dec 13, 2024 •

edited

Loading

adarshs1310 commented Dec 16, 2024

adarshs1310 commented Dec 17, 2024 •

edited

Loading

alexanderguzhva Dec 17, 2024

adarshs1310 Dec 17, 2024

alexanderguzhva commented Dec 18, 2024

	target_compile_options(utils_sse PRIVATE -msse4.2 -mpopcnt)
	target_compile_options(utils_avx PRIVATE -mfma -mf16c -mavx2 -mpopcnt)
	target_compile_options(utils_avx512 PRIVATE -mfma -mf16c -mavx512f -mavx512dq
	-mavx512bw -mpopcnt -mavx512vl)

Enable SVE Support for L2 Metric Computation in FP32 #969

Are you sure you want to change the base?

Enable SVE Support for L2 Metric Computation in FP32 #969

Conversation

adarshs1310 commented Nov 29, 2024 • edited Loading

sre-ci-robot commented Nov 29, 2024

sre-ci-robot commented Nov 29, 2024

mergify bot commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adarshs1310 commented Nov 30, 2024

adarshs1310 commented Nov 30, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

alexanderguzhva commented Dec 3, 2024

adarshs1310 commented Dec 3, 2024

adarshs1310 commented Dec 9, 2024

foxspy commented Dec 9, 2024

alexanderguzhva commented Dec 9, 2024 • edited Loading

adarshs1310 commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adarshs1310 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

adarshs1310 commented Dec 16, 2024

adarshs1310 commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderguzhva commented Dec 18, 2024

adarshs1310 commented Nov 29, 2024 •

edited

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

alexanderguzhva commented Dec 9, 2024 •

edited

Loading

adarshs1310 Dec 13, 2024 •

edited

Loading

adarshs1310 commented Dec 17, 2024 •

edited

Loading