Add missing headers in faiss/[gpu/]CMakeLists.txt #1666
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
|
@beauby @mdouze @wickedfoo Sidenote: the build times basically doubled across the board from 1.6.5 to 1.7.0 - not sure if you're seeing similar increases here, but just FYI. |
|
LGTM, although I am not the cmake expert here. Will import it to the FB review system. |
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.
This was referenced Feb 12, 2021
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
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 7, 2021
Summary: Some small fixes from conda-forge/faiss-split-feedstock#46; some missing (and unsorted) headers like in #1666, and a missing `<algorithm>` include like in #1895 Pull Request resolved: #1933 Reviewed By: mdouze Differential Revision: D28931009 Pulled By: beauby fbshipit-source-id: c8a9e52a9237dc0bb87664441a76a5db47cc821a
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 preparing conda-forge/faiss-split-feedstock#26, I grepped for the expected headers based on the files in the repo, à la:
Doing so uncovered that there were some headers missing (apparently) in
CMakeLists.txt, namely:It's possible that they were left out intentionally, but I didn't see something that would make me think so e.g. in
ResultHandler.h.While I was at it, I decided to order the filenames consistently (alphabetically, except for the increasing bit-sizes for blockselect/warpselect, as is already the case for
impl/scan/IVFInterleaved<x>.cu), but of course, those commits could easily be dropped.By reviewing the commits separately, it should be clear (for the first two) from the equal number of deletions/insertions (and the simple diff) that this is just a reshuffle. The only additions are in the last commit.