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

Support SIMD Histogram Subroutines on aarch64 #2447

Closed
wants to merge 2 commits into from

Conversation

wx257osn2
Copy link
Contributor

Currently SIMD histogram subroutines are written with simdlib and AVX2 intrinsics .
This PR adds some functions to simdlib and removes AVX2 intrinsics from SIMD histogram subroutines, so faiss with this PR can execute histogram using ARM SIMD on aarch64.

@wx257osn2 wx257osn2 marked this pull request as ready for review September 1, 2022 21:09
@wx257osn2
Copy link
Contributor Author

wx257osn2 commented Sep 1, 2022

Ah, ok, this PR depends on #2448. I'll re-send this PR after #2448 will be merged. Sorry for sending you an undesirable notification... 🙇

@wx257osn2 wx257osn2 closed this Sep 1, 2022
@wx257osn2 wx257osn2 reopened this Sep 8, 2022
@mdouze
Copy link
Contributor

mdouze commented Sep 13, 2022

The code looks good.
However, the histogram functionality is not used in the current version of the FastScan implementation, therefore I am not sure there is test coverage for the function. Could you check that? And otherwise add a test?

@mdouze
Copy link
Contributor

mdouze commented Sep 13, 2022

Ah right, it is tested in
https://github.com/facebookresearch/faiss/blob/151e3d7be54aec844b6328dc3e7dd0b83fcfa5bc/tests/test_partition.py
so that looks fine.

@facebook-github-bot
Copy link
Contributor

@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mdouze
Copy link
Contributor

mdouze commented Oct 6, 2022

I have trouble landing this, I may need to make a new PR

@wx257osn2
Copy link
Contributor Author

@mdouze Is there any action that I should do, like closing this and making a new PR with a new branch, or something like that?

@mdouze
Copy link
Contributor

mdouze commented Oct 7, 2022

If you could rebase this PR on the latest Faiss it would be helpful. Thansk.

@wx257osn2
Copy link
Contributor Author

@mdouze Sorry, I thought it was rebased by the "Update branch" button until just now (the button had been MERGING main branch into this branch, but not rebasing this branch into main) 🙇 I have rebased it, so would you import it again? thanks

@facebook-github-bot
Copy link
Contributor

@mdouze has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mdouze
Copy link
Contributor

mdouze commented Mar 24, 2023

Thanks, second attempt...

@facebook-github-bot
Copy link
Contributor

@mdouze merged this pull request in 5dcd282.

@wx257osn2 wx257osn2 deleted the histogram-neon branch March 24, 2023 13:05
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.

3 participants