-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add sve targets #2886
Add sve targets #2886
Conversation
Please don't add a faiss/python/swigfaiss_sve.swig file. |
Oh, sorry. I missed but that has been copied at this line. I removed the file and added the path on |
091b0f7
to
5a3d8ea
Compare
96f35db
to
d7c27ba
Compare
🤨 |
Ah, #2917, OK. |
@mdouze How about the current status of this PR? |
So the diff only changes the compilation flags, it does not add VSE specific SIMD implementations, right? |
In this PR faiss uses SVE only with auto vectorized functions like |
As I wrote before,
It will include SVE implmemtations of |
@mdouze IMO the PRs should be separated, but I'm willing to include the commits of performance improvement in this PR if you want it. How would you like it? |
Sorry for being a bit slow to react. |
@mdouze OK. When you will want my action like:
please feel free to send me some comments. Anyway, I will wait the checking for a while. Thanks. |
@mdouze and @vorj is there any update on adding SVE support and do you guys still have plans to add it? I saw some discussion on the other PR and there was no activity since a while. Basically, we were looking for some optimization to Scalar Quantization(specifically SQfp16) on ARM like AVX2 on x86. Also, please let us know if you need any help to run tests for SVE support. We have bandwidth and resources to run tests. Thanks! |
@naveentatikonda I am just a contributor not employed by Meta, so actually I don't know the plans on this (official faiss) repository. However, as I told above, I have further patches to improve performance more, and I will create PR when this merged. |
@mdouze Did you get a chance to look into my question? |
OK so I think a way to move forward is to accept this PR but not cover it with CI.
Is there a doc somewhere that shows what current and future ARM implementaitons support SVE ? Thanks |
Would you mind rebasing on the latest Faiss so that I can import it to the internal Faiss version? |
I can assist and review the code, if needed |
@vorj Thanks! We will be trying this shortly. Meanwhile, I restarted the failed build, there was a transient error and it should be fixed now. |
That's not surprising because what you are saying is like that faiss built with in the issue, Arm SVE is the extension of ARMv8 ISA, and AWS Graviton2 doesn't support it. P.S. I'm not a Meta employee (unfortunately), so I can't see the internal URLs if you link it |
@vorj Thanks! I'll let @mengdilin confirm but I believe this is resolved when it was retried with r8g.large (ARMv9 / Neoverse V2) which does support SVE and I believe SVE2 (something that might be of interest I guess). |
Yes, Graviton >= 3 (including r7g.large and r8g.large) can solve above issue, so please take a try. When you will meet other problems, please let me know. BTW, this PR activates only SVE but not SVE2, so when we want to use SVE2 we need to another PR (and finally it will generate another binary named |
Yeah, @mengdilin tried with r8g and it worked. Re: SVE2, is that something you would be interested in contributing? I think it’ll be much easier and quicker to merge that one in next. |
Yes, but currently I have not caught up that yet, so it might not be in the near future. |
@mengdilin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mengdilin merged this pull request in 4eeaa42. |
Summary: related: facebookresearch#2884 This PR contains below changes: - Add new optlevel `sve` - ARM SVE is _extension_ of ARMv8, so it should be treated similar to AVX2 IMO - Add targets for ARM SVE, `faiss_sve` and `swigfaiss_sve` - These targets will be built when you give `-DFAISS_OPT_LEVEL=sve` at build time - Design decision: Don't fix SVE register length. - The python package of faiss is "fat binary" (for example, the package for avx2 contains `_swigfaiss_avx2.so` and `_swigfaiss.so`) - SVE is scalable instruction set (= doesn't fix vector length), but actually we can specify the vector length at compile time. - [with `-msve-vector-length=` option](https://developer.arm.com/documentation/101726/4-0/Coding-for-Scalable-Vector-Extension--SVE-/SVE-Vector-Length-Specific--VLS--programming) - When this option is specified, the binary can't work correctly on the CPU which has other vector length rather than specified at compile time - When we use fixed vector length, SVE-supported faiss python package will contain 7 shared libraries like `_swigfaiss.so` , `_swigfaiss_sve.so` , `_swigfaiss_sve128.so` , `_swigfaiss_sve256.so` , `_swigfaiss_sve512.so` , `_swigfaiss_sve1024.so` , and `_swigfaiss_sve2048.so` . The package size will be exploded. - For these reason, I don't specify the vector length at compile time and `faiss_sve` detects the vector length at run time. - Add a mechanism of detecting ARM SVE on runtime environment and importing `swigfaiss_sve` dynamically - Currently it only supports Linux, but there is no SVE environment with non-Linux OS now, as far as I know NOTE: I plan to make one more PR about add some SVE implementation after this PR merged. This PR only contains adding sve target. Pull Request resolved: facebookresearch#2886 Reviewed By: ramilbakhshyiev Differential Revision: D60386983 Pulled By: mengdilin fbshipit-source-id: 7e66162ee53ce88fbfb6636e7bf705b44e6c3282
Summary: #2943 had removed about SVE information (added on #2886 ) on the installation document. This PR fixes it. This PR changes only the document, so it doesn't affect software behavior. Pull Request resolved: #3915 Reviewed By: asadoughi Differential Revision: D63967842 Pulled By: ramilbakhshyiev fbshipit-source-id: ce0a0bfe591cb75b504cdf6362b5e8ed156928d5
Summary: related: facebookresearch#2884 This PR contains below changes: - Add new optlevel `sve` - ARM SVE is _extension_ of ARMv8, so it should be treated similar to AVX2 IMO - Add targets for ARM SVE, `faiss_sve` and `swigfaiss_sve` - These targets will be built when you give `-DFAISS_OPT_LEVEL=sve` at build time - Design decision: Don't fix SVE register length. - The python package of faiss is "fat binary" (for example, the package for avx2 contains `_swigfaiss_avx2.so` and `_swigfaiss.so`) - SVE is scalable instruction set (= doesn't fix vector length), but actually we can specify the vector length at compile time. - [with `-msve-vector-length=` option](https://developer.arm.com/documentation/101726/4-0/Coding-for-Scalable-Vector-Extension--SVE-/SVE-Vector-Length-Specific--VLS--programming) - When this option is specified, the binary can't work correctly on the CPU which has other vector length rather than specified at compile time - When we use fixed vector length, SVE-supported faiss python package will contain 7 shared libraries like `_swigfaiss.so` , `_swigfaiss_sve.so` , `_swigfaiss_sve128.so` , `_swigfaiss_sve256.so` , `_swigfaiss_sve512.so` , `_swigfaiss_sve1024.so` , and `_swigfaiss_sve2048.so` . The package size will be exploded. - For these reason, I don't specify the vector length at compile time and `faiss_sve` detects the vector length at run time. - Add a mechanism of detecting ARM SVE on runtime environment and importing `swigfaiss_sve` dynamically - Currently it only supports Linux, but there is no SVE environment with non-Linux OS now, as far as I know NOTE: I plan to make one more PR about add some SVE implementation after this PR merged. This PR only contains adding sve target. Pull Request resolved: facebookresearch#2886 Reviewed By: ramilbakhshyiev Differential Revision: D60386983 Pulled By: mengdilin fbshipit-source-id: 7e66162ee53ce88fbfb6636e7bf705b44e6c3282
…#3915) Summary: facebookresearch#2943 had removed about SVE information (added on facebookresearch#2886 ) on the installation document. This PR fixes it. This PR changes only the document, so it doesn't affect software behavior. Pull Request resolved: facebookresearch#3915 Reviewed By: asadoughi Differential Revision: D63967842 Pulled By: ramilbakhshyiev fbshipit-source-id: ce0a0bfe591cb75b504cdf6362b5e8ed156928d5
related: #2884
This PR contains below changes:
sve
faiss_sve
andswigfaiss_sve
-DFAISS_OPT_LEVEL=sve
at build time_swigfaiss_avx2.so
and_swigfaiss.so
)-msve-vector-length=
option_swigfaiss.so
,_swigfaiss_sve.so
,_swigfaiss_sve128.so
,_swigfaiss_sve256.so
,_swigfaiss_sve512.so
,_swigfaiss_sve1024.so
, and_swigfaiss_sve2048.so
. The package size will be exploded.faiss_sve
detects the vector length at run time.swigfaiss_sve
dynamicallyNOTE: I plan to make one more PR about add some SVE implementation after this PR merged. This PR only contains adding sve target.