make setup.py win+avx2 compatible #1682
Closed
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
h-vetinari
changed the title
h-vetinari
force-pushed
the
ext_suffix
branch
from
f171337
to
3e44344
Compare
h-vetinari
changed the title
h-vetinari
force-pushed
the
ext_suffix
branch
from
3e44344
to
7322705
Compare
h-vetinari
force-pushed
the
ext_suffix
branch
from
ded199e
to
b67bbe2
Compare
|
The failing test seems to be flaky. |
|
For this, I'd be more inclined to keep the CMake config as is, and just deal with it in |
Understandable. I'd suggest to keep the changes in |
h-vetinari
force-pushed
the
ext_suffix
branch
from
78a95d9
to
87426b6
Compare
h-vetinari
changed the title
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
h-vetinari
force-pushed
the
ext_suffix
branch
from
87426b6
to
5ce2103
Compare
|
@beauby |
There was a problem hiding this comment.
@beauby 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 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
4 tasks
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.
While working on conda-forge/faiss-split-feedstock#27, it turned out I needed
to patch
setup.pyanyway. In order to unify how the extension of the built lib is set, I fell backto 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
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.INFOin case of an import failure for AVX2, but since it's a setup file,I thought
printwould actually be more useful. Happy to change or remove if desired.