Skip to content
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

Initial ppc64le support #162

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Initial ppc64le support #162

merged 1 commit into from
Dec 11, 2023

Conversation

mgiessing
Copy link
Contributor

@mgiessing mgiessing commented Oct 24, 2023

This PR aims to support the build process of knowhere for ppc64le architecture: #161

@sre-ci-robot
Copy link
Collaborator

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

@mergify
Copy link

mergify bot commented Oct 24, 2023

@mgiessing 🔍 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!.

@alexanderguzhva
Copy link
Collaborator

/lgtm

@alexanderguzhva
Copy link
Collaborator

@mgiessing could you please squash your commits? It is a preferred way for knowledge to have a single commit for a PR. Thanks

@mgiessing
Copy link
Contributor Author

@alexanderguzhva Just squashed the commits - thanks!

@alexanderguzhva
Copy link
Collaborator

/lgtm

@alexanderguzhva
Copy link
Collaborator

@mgiessing Hey, would you please be able to fix the formatting (check Pre-commit stage)? Thanks!

@mgiessing
Copy link
Contributor Author

@alexanderguzhva I'm not sure what's still wrong
I installed the pre-commit hook, run the clang-formatting (which apparently also changed formatting of 2 other files) and when pushed it everything looked fine. Nonetheless gh actions complains about it :/

@liliu-z
Copy link
Collaborator

liliu-z commented Dec 7, 2023

Hi @mgiessing Instead of explicitly add support for ppc64le, is it possible to make this changes under a default fall back solution? Since there is no ppc64le specific change here.

Also can you help take a look at the pre commit error in the github action and fix the format?

Thanks

@mgiessing
Copy link
Contributor Author

mgiessing commented Dec 7, 2023

@liliu-z That would be one option, however on the other hand I'm also thinking about adding vector intrinsic code for the distance calculation (i.e. adding distances_vsx.cc & distances_vsx.h. However that would probably take some more time to accomplish.

@liliu-z
Copy link
Collaborator

liliu-z commented Dec 8, 2023

@liliu-z That would be one option, however on the other hand I'm also thinking about adding vector intrinsic code for the distance calculation (i.e. adding distances_vsx.cc & distances_vsx.h. However that would probably take some more time to accomplish.

Sure, Thanks for this.
Before merging in, can you help:

  1. Revert the format change for include/knowhere/dataset.h and benchmark/benchmark_base.h to pass the pre-commit check. Sorry about this inconsistent experience it brings. We will take a look but we can bypass it for now.
  2. Remake the commit message by git --amend -s to add the signature.

Thanks

@liliu-z
Copy link
Collaborator

liliu-z commented Dec 8, 2023

/kind enhancement

@liliu-z
Copy link
Collaborator

liliu-z commented Dec 8, 2023

/approve

@sre-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liliu-z, mgiessing

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:

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

@yellow-shine
Copy link
Collaborator

yellow-shine commented Dec 8, 2023

this solves: #161

This was referenced Dec 8, 2023
@mgiessing
Copy link
Contributor Author

@liliu-z this is very weird. If I follow the Contributing guidelines and install the pre-commit hook locally I get a FAILED because of the two mentioned files:

$ git push
check for added large files..............................................Passed
check for merge conflicts................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1

benchmark/benchmark_base.h
====================
--- original

+++ formatted

@@ -188,10 +188,10 @@

     void
     free_all() {
         if (xb_ != nullptr) {
-            delete[](float*) xb_;
+            delete[] (float*)xb_;
         }
         if (xq_ != nullptr) {
-            delete[](float*) xq_;
+            delete[] (float*)xq_;
         }
         if (gt_radius_ != nullptr) {
             delete[] gt_radius_;
include/knowhere/dataset.h
====================
--- original

+++ formatted

@@ -36,25 +36,25 @@

             {
                 auto ptr = std::get_if<0>(&x.second);
                 if (ptr != nullptr) {
-                    delete[] * ptr;
+                    delete[] *ptr;
                 }
             }
             {
                 auto ptr = std::get_if<1>(&x.second);
                 if (ptr != nullptr) {
-                    delete[] * ptr;
+                    delete[] *ptr;
                 }
             }
             {
                 auto ptr = std::get_if<2>(&x.second);
                 if (ptr != nullptr) {
-                    delete[] * ptr;
+                    delete[] *ptr;
                 }
             }
             {
                 auto ptr = std::get_if<3>(&x.second);
                 if (ptr != nullptr) {
-                    delete[](char*)(*ptr);
+                    delete[] (char*)(*ptr);
                 }
             }
         }

error: failed to push some refs to 'https://github.com/mgiessing/knowhere'

I'm wondering you don't get this, FYI I use clang-format 17.0.6:

$ clang-format --version
Homebrew clang-format version 17.0.6

Nevertheless I try to uninstall the hook and push again

Signed-off-by: mgiessing <[email protected]>

Added ppc64le hook (without intrinsics)

Signed-off-by: mgiessing <[email protected]>

pre-commit fix

Signed-off-by: mgiessing <[email protected]>

Revert "Initial ppc64le support"

This reverts commit 3e77eee.

Fixes for ppc64le

Signed-off-by: mgiessing <[email protected]>
@liliu-z
Copy link
Collaborator

liliu-z commented Dec 11, 2023

/lgtm
Thanks~

@sre-ci-robot sre-ci-robot merged commit 968157c into zilliztech:main Dec 11, 2023
9 checks passed
@sumitd2
Copy link

sumitd2 commented Apr 16, 2024

@mgiessing Do you have any update on the SIMD optimizations? Thanks.

@mgiessing
Copy link
Contributor Author

I briefly had a look into manually creating VSX code however the auto-vectorizer seems to do a pretty good job and there was no real benefit when I compared handwritten VSX code (which might not be perfect of course) vs. what the compiler generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants