Add optimized distiance functions for PowerPC

[carll99]

Files src/simd/distances_powerpc.cc, src/simd/distances_powerpc.h are added with powerpc optimized versions of the various functions.

File src/simd/hook.cc is updated to point to the new PowerPC versions of the functions.

[sre-ci-robot]

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

[mergify]

@carll99 🔍 Important: PR Classification Needed!

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

1. If you're fixing a bug, label it as kind/bug.
2. For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
3. Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
4. 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!.

[carll99]

Based on the comments I got from generating the PR, it sounds like I am supposed to set the kind label? Not sure how that should have been done. Secondly, this adds specific PowerPC support which I believe makes it kind/feature?

[alexanderguzhva]

Facebook, not Zilliz in a license header?

[alexanderguzhva]

please call the function fvec_L2sqr_ppc, not fvec_L2sqr_ref_ppc.
Same for all other functions.

[alexanderguzhva]

for (size_t i = 0; i < base; i += FLOAT_VEC_SIZE) {
same comment applies to loops in other functions

[alexanderguzhva]

please get rid of this local variable.
same comment applies to other functions

[alexanderguzhva]

for (size_t i = base; i < d; i++) {
same comment applies to loops in other functions

[alexanderguzhva]

I just want to confirm that all knowhere unit tests passed, correct?

[carll99]

Alexander: Thank you for reviewing the code. I will make the updates and we will retest the patch. Per your comment at the bottom, yes the changes were tested on a PowerPC processor to verify the code compiles, executes and produces the same results as the reference functions. Just want to verify the procedure for posting the updated version. I am assuming that once I have resynched my github with the latest upstream code and my updated patch that I would create a new pull request. Not sure if there is anything I need to do to cancel the initial pull request. Thanks for the help. Carl Love

________________________________ From: Alexander Guzhva ***@***.***> Sent: Wednesday, October 16, 2024 4:23 AM To: zilliztech/knowhere ***@***.***> Cc: Carl Love ***@***.***>; Mention ***@***.***> Subject: [EXTERNAL] Re: [zilliztech/knowhere] Add optimized distiance functions for PowerPC (PR #894) @ alexanderguzhva commented on this pull request. In src/simd/distances_powerpc. cc: > @@ -0,0 +1,540 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. Facebook, not Zilliz in a license header? In src/simd/distances_powerpc. cc: > @alexanderguzhva commented on this pull request.

________________________________ In src/simd/distances_powerpc.cc<#894 (comment)>:

@@ -0,0 +1,540 @@

+/** + * Copyright (c) Facebook, Inc. and its affiliates. Facebook, not Zilliz in a license header?

________________________________ In src/simd/distances_powerpc.cc<#894 (comment)>:

+#if defined(__powerpc__)

+ +#include <altivec.h> /* Required for the Power GCC built-ins */ + +#include "distances_powerpc.h" + +#include <cmath> + +#define FLOAT_VEC_SIZE 4 +#define INT32_VEC_SIZE 4 +#define INT8_VEC_SIZE 16 + +namespace faiss { + +float +fvec_L2sqr_ref_ppc(const float* x, const float* y, size_t d) { please call the function fvec_L2sqr_ppc, not fvec_L2sqr_ref_ppc. Same for all other functions.

________________________________ In src/simd/distances_powerpc.cc<#894 (comment)>:

+float

+fvec_L2sqr_ref_ppc(const float* x, const float* y, size_t d) { + /* Vector implmentaion uses vector size of FLOAT_VEC_SIZE. If the input + array size is not a power of FLOAT_VEC_SIZE, do the remaining elements + in scalar mode. */ + size_t i; + float res = 0; + size_t base; + + vector float *vx, *vy; + vector float vtmp = {0, 0, 0, 0}; + vector float vres = {0, 0, 0, 0}; + + base = (d / FLOAT_VEC_SIZE) * FLOAT_VEC_SIZE; + + for (size_t i = 0; i < base; i = i + FLOAT_VEC_SIZE) { for (size_t i = 0; i < base; i += FLOAT_VEC_SIZE) { same comment applies to loops in other functions

________________________________ In src/simd/distances_powerpc.cc<#894 (comment)>:

+#include "distances_powerpc.h"

+ +#include <cmath> + +#define FLOAT_VEC_SIZE 4 +#define INT32_VEC_SIZE 4 +#define INT8_VEC_SIZE 16 + +namespace faiss { + +float +fvec_L2sqr_ref_ppc(const float* x, const float* y, size_t d) { + /* Vector implmentaion uses vector size of FLOAT_VEC_SIZE. If the input + array size is not a power of FLOAT_VEC_SIZE, do the remaining elements + in scalar mode. */ + size_t i; please get rid of this local variable. same comment applies to other functions

________________________________ In src/simd/distances_powerpc.cc<#894 (comment)>:

+ vector float *vx, *vy;

+ vector float vtmp = {0, 0, 0, 0}; + vector float vres = {0, 0, 0, 0}; + + base = (d / FLOAT_VEC_SIZE) * FLOAT_VEC_SIZE; + + for (size_t i = 0; i < base; i = i + FLOAT_VEC_SIZE) { + vx = (vector float *)(&x[i]); + vy = (vector float *)(&y[i]); + + vtmp = vx[0] - vy[0]; + vres += vtmp * vtmp; + } + + /* Handle any remaining data elements */ + for (i = base; i < d; i++) { for (size_t i = base; i < d; i++) { same comment applies to loops in other functions

________________________________ In src/simd/distances_powerpc.cc<#894 (comment)>:

@@ -0,0 +1,540 @@

+/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#if defined(__powerpc__) I just want to confirm that all knowhere unit tests passed, correct? — Reply to this email directly, view it on GitHub<#894 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BGQHXSAEEBENZE7M5GXPCYTZ3ZEBZAVCNFSM6AAAAABP7THYPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNZSGA4TQMZXGQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[alexanderguzhva]

@carll99 just force push the new changes to override this PR with a better version

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.19%. Comparing base (3c46f4c) to head (609931d).
Report is 253 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##           main     #894       +/-   ##
=========================================
+ Coverage      0   74.19%   +74.19%     
=========================================
  Files         0       82       +82     
  Lines         0     6600     +6600     
=========================================
+ Hits          0     4897     +4897     
- Misses        0     1703     +1703     

see 82 files with indirect coverage changes

[liliu-z]

Hi @carll99
Due to the limited resource, we actually doesn't officially support PowerPC, which means the official release and new features will not take care of the PowerPC specific scenario.
However, we are welcome for the contribution and maintain from the community. Do you mind to share with us more about the use case to help us know more about the importance of PowerPC?

[alexanderguzhva]

@carll99 please accept DCO and run clang-format and I'll lgtm the PR. Thanks.

[carll99]

I updated the patch and the team is working on retesting it.

I forwarded the comment from liliu-z to the PowerPC and we will work on the response.

The DCO, looks like the description for DCO says the project wants a sign-off line. I will include that with the next version.

I will check how to run the clang-format tool. I did a quick google to see what it is.

Thanks for the feedback.

[carll99]

Alexander: Sorry for the delay in responding. We had a number of machine issues with took several weeks to resolve. I updated the patch per the comments from your last review. I ran "pre-commit install --hook-type pre-commit --hook-type pre-push". I manually ran the clang-format with the command "git clang-format HEAD~1" at the git tree head. It edited a number of lines. I reviewed the changes and then included the changes into the patch. The patch was tested and the results verified. The one thing I am not sure how to do is accepting the DCO? I believe the DCO stands for Developer Certificate of Origin. I have been looking around to see if I can find instructions on the DCO but I am not finding them. Can you point me to how to do the DCO acceptance? Thanks. I think we just need to specify who wrote the code. Not sure if we can say IBM wrote it or if we need to specify the specific people in IBM. I did 95% of the code but then Anjil made a few changes to get the code to compile. The other comment that I got lilu-z "Do you mind to share with us more about the use case to help us know more about the importance of PowerPC?" The IBM Watsonx product uses Milvus. Milvus uses the Knowhere code. So IBM is very interested in ensuring the performance of the Knowhere code is optimized for PowerPC. We have a number of people working on improving the various AI code bases like Knowhere and FAISS. Not sure if that is sufficient or do you need more? Carl

…

________________________________ From: Alexander Guzhva ***@***.***> Sent: Monday, October 21, 2024 7:02 AM To: zilliztech/knowhere ***@***.***> Cc: Carl Love ***@***.***>; Mention ***@***.***> Subject: [EXTERNAL] Re: [zilliztech/knowhere] Add optimized distiance functions for PowerPC (PR #894) @ carll99 please accept DCO and run clang-format and I'll lgtm the PR. Thanks. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned. Message ID: <zilliztech/knowhere/pull/894/c2426778118@ github. com> @carll99<https://github.com/carll99> please accept DCO and run clang-format and I'll lgtm the PR. Thanks. — Reply to this email directly, view it on GitHub<#894 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BGQHXSC6UUYYOZ6L4YKP7NLZ4UCQ3AVCNFSM6AAAAABP7THYPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRWG43TQMJRHA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[alexanderguzhva]

@carll99 Please carefully read the error message https://github.com/zilliztech/knowhere/pull/894/checks?check_run_id=32817538304, all instructions are there
Also, would you please update a PR to include your changes?
Thanks

[carll99]

I have updated my changes for PowerPC. They have been tested on a Power10 to verify they compile and the results are correct.

Per the questions earlier about the IBM use case for this code.

The IBM Watsonx product uses Milvus.  Milvus uses the Knowhere code.  So IBM is very interested in ensuring the performance of the Knowhere code is optimized for PowerPC.  We have a number of people at IBM working on improving the various AI code bases like Knowhere and FAISS. This is an ongoing project. IBM is putting a number of resources into these projects. They are all fundamental to the continued development of the IBM Watsonx product. I hope that helps answer your question.

Finally, sorry about the slow response getting the updated code. We had a number of our lab machines fail. They were down for a couple of weeks. Also, the testing and verification of the patch took a lot longer than expected. My apologies for the slow turn around on the changes.

[alexanderguzhva]

/lgtm

[foxspy]

/kind improvement

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carll99, foxspy

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [foxspy]

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