Win+AVX2 compat #1681
Closed
Win+AVX2 compat #1681
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thankfully, __builtin_popcount / __builtin_popcountl are very clearly distinguishable visually.
h-vetinari
force-pushed
the
win_avx2_compat
branch
from
815fc76
to
4123c02
Compare
h-vetinari
commented
Feb 12, 2021
faiss/impl/ScalarQuantizer.cpp
Outdated
| @@ -40,7 +41,7 @@ namespace faiss { | |||
| ********************************************************************/ | |||
|
|
|||
| #ifdef __AVX2__ | |||
| #ifdef __F16C__ | |||
| #if defined(__F16C__) || defined(_MSC_VER) | |||
There was a problem hiding this comment.
Realizing now that __F16__ could be set in platform_macros.h like __SSEx__...
|
Interesting that this manages to provoke a failure... 🤔 The only change that is even visible on a non-windows system is the |
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 12, 2021
Summary: This patch is to provide log information in case of failing to load the AVX2-enabled lib, and also to log the successful loading of the libraries. Otherwise, the programmatic detection of which library was loaded also needs to take into account which other log statements were executed, if the original import failed due to an exception, cf. the tests in conda-forge/faiss-split-feedstock#27 Finally, this reduces the number of import statements by one - one non-AVX2 fallback is enough. Related to #1600, #1680, #1681, #1682. Pull Request resolved: #1683 Reviewed By: mdouze Differential Revision: D26424333 Pulled By: beauby fbshipit-source-id: 16beddec7e0c098b913a7f5420cbb02d1cf515ad
|
Looks good to me. |
There was a problem hiding this comment.
@mdouze has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 17, 2021
Summary: While working on conda-forge/faiss-split-feedstock#27, it turned out I needed to patch `setup.py` anyway. In order to unify how the extension of the built lib is set, I fell back to another patch that would/will become necessary if faiss ever wants to support PyPy (see discussion in conda-forge/faiss-split-feedstock#22). It would be nice if this was done natively by CMake, but as far as I can tell from https://gitlab.kitware.com/cmake/cmake/-/issues/21070, cmake is not likely to do that right away. I didn't particularly expect this patch to be upstreamed (especially if there is no interest for PyPy support, for example), but beauby [invited](conda-forge/faiss-split-feedstock#27 (comment)) me to post it so here goes (plus necessary adaptations to the conda recipes) Related to #1600, #1680, #1681 PS. I thought about using `logger.INFO` in case of an import failure for AVX2, but since it's a setup file, I thought `print` would actually be more useful. Happy to change or remove if desired. Pull Request resolved: #1682 Reviewed By: wickedfoo Differential Revision: D26484393 Pulled By: beauby fbshipit-source-id: 6cd2598838c4070dbf83d6f27ce15ce9faa6bf20
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 18, 2021
Summary: Trying to compile windows for AVX2 in conda-forge/faiss-split-feedstock#27 (after #1600) surfaced a bunch of things (#1680, #1681, #1682), but the most voluminous problem was MSVC being much worse at dealing with operator overloads and casts around `__m128` / `__m256`. This lead to loads of errors that looked as follows: ``` [...]\faiss\utils\distances_simd.cpp(411): error C2676: binary '+=': '__m128' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(440): error C2676: binary '-': '__m256' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(441): error C2676: binary '*': 'const __m256' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(446): error C2676: binary '+=': '__m128' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(451): error C2676: binary '-': '__m128' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(452): error C2676: binary '*': 'const __m128' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(459): error C2676: binary '-': '__m128' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(460): error C2676: binary '*': '__m128' does not define this operator or a conversion to a type acceptable to the predefined operator [...]\faiss\utils\distances_simd.cpp(471): error C2440: '<function-style-cast>': cannot convert from '__m256i' to '__m256' ``` I've followed https://software.intel.com/sites/landingpage/IntrinsicsGuide/ to try to replace everything correctly, but this will surely require close review, because I'm not sure how well these code-paths are checked by the test suite. In any case, with the commits from #1600 #1666 #1680 #1681 #1682, I was able to build `libfaiss` & `faiss` for AVX2 on windows (while remaining "green" on linux/osx, both with & without AVX2). Sidenote: the issues in the last commit (26fc7cf) were uncovered by adding the `__SSE3__` compat macros in #1681. Pull Request resolved: #1684 Test Plan: buck test //faiss/tests/... Reviewed By: beauby Differential Revision: D26454443 Pulled By: mdouze fbshipit-source-id: 70df0818e357f1ecea6a056d619618df0236e0eb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upstreaming patches from conda-forge/faiss-split-feedstock#27, follow-up (sorta) to #1600.
All these should be fairly uncontroversial, I think (mostly just oversights or stuff that never got
triggered on windows due to not having #1680 so far).
Things work without
#include <faiss/impl/platform_macros.h>, but I preferred to be explicit here,because the
__SSEx__macros (that are used in the affected files) are only defined there for windows.