make AVX2-detection platform-independent #1600

h-vetinari · 2020-12-30T01:20:19Z

In the context of conda-forge/faiss-split-feedstock#23, I discussed with some of the conda-folks how we should support AVX2 (and potentially other builds) for faiss. In the meantime, we'd like to follow the model that faiss itself is using (i.e. build with AVX2 and without and then load the corresponding library at runtime depending on CPU capabilities).

Since windows support for this is missing (and the other stuff is also non-portable in loader.py), I chased down numpy.distutils.cpuinfo, which is pretty outdated, and opened: numpy/numpy#18058

While the private API is obviously something that could change at any time, I still think it's better than platform-dependent shenanigans.

Opening this here to ideally upstream this right away, rather than carrying patches in the conda-forge feedstock.

TODO:

adapt conda recipe for windows in this repo to also build avx2 version

facebook-github-bot · 2020-12-30T01:20:24Z

Hi @h-vetinari!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2020-12-30T01:45:56Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

h-vetinari · 2021-01-11T22:37:49Z

@wickedfoo @mdouze
I'd like to build avx2 for windows too in conda-forge (see OP), and having your approval on this approach would make me more confident that I'm not going against your ideas somehow. PTAL

mdouze · 2021-01-12T08:00:58Z

That would be great! Adding @beauby for the status of AVX2 on Windows (not sure if it is tested at all).
NB that in the Faiss Circle CI we do not test AVX2 because not all CI machines support it.
This means that a lot of codepaths are not exercised. For AVX2 on Linux we rely on the FB internal tests.

h-vetinari · 2021-01-12T08:35:55Z

This means that a lot of codepaths are not exercised.

I was under the impression that it's just a compiler flag that's changing between building for avx2 or not...?

mdouze · 2021-01-18T08:49:52Z

faiss/python/loader.py



 logger = logging.getLogger(__name__)

 try:
-    instr_set = instruction_set()
-    if instr_set == "AVX2":
+    has_AVX2 = supports_AVX2()


I would like to keep the instruction_set name here generic since Faiss also compiles on ARM and PPC.

h-vetinari · 2021-01-20T18:22:15Z

Responded to the feedback @mdouze, PTAL :)

facebook-github-bot

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

facebook-github-bot

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

mdouze

small comment.

mdouze · 2021-01-29T10:48:39Z

faiss/python/loader.py

+    {"VSX": True, "VSX2": False, ...}
+    >>> instruction_set()  # for ARM
+    {"NEON": True, "ASIMD": False, ...}
+    """


I know this is constrained on the numpy side but what about returning a set with the flags set to True?

For example, below the has_AVX2 check will fail on ARM with KeyError.

Good point about the key error. I think the cleanest fix would just be to use .get("AVX2", False) on the output, which is fine even if some keys are not there.

Alternatively, we could "pad" the dictionary with the keys from the other architectures with value False. I don't like this idea though, because we'd be duplicating the list of instruction sets compared to numpy

We could also return a set in every case (in the case of numpy, selecting the keys for which the value is True).

Sure, that's a good idea as well.

Thanks for using the set. Will get it through the FB internal checks.

It is a set of instruction sets, and "instruction_set" is not great for that

facebook-github-bot

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

facebook-github-bot · 2021-02-03T16:02:27Z

@mdouze merged this pull request in 16b4e88.

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

Summary: 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. Pull Request resolved: #1681 Reviewed By: beauby Differential Revision: D26454427 Pulled By: mdouze fbshipit-source-id: 345e0ef45888f338e71bba004454a701572f9afb

Summary: Upstreaming patches from conda-forge/faiss-split-feedstock#27, follow-up (sorta) to #1600. Not sure if there are more CMake-native tricks to use here, but given that the flags don't have an equivalent on the MSVC side, I think this approach is reasonable. Without this patch, we would get: ``` cl : Command line warning D9002: ignoring unknown option '-mavx2' cl : Command line warning D9002: ignoring unknown option '-mfma' cl : Command line warning D9002: ignoring unknown option '-mf16c' cl : Command line warning D9002: ignoring unknown option '-mpopcnt' ``` Pull Request resolved: #1680 Reviewed By: wickedfoo Differential Revision: D26484347 Pulled By: beauby fbshipit-source-id: 2803132f2d81fe37dc494fc4c824b6e240ae973b

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

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

make AVX2-detection platform-independent

62519d8

h-vetinari force-pushed the master branch from 3be37a8 to 62519d8 Compare December 30, 2020 01:34

h-vetinari closed this Dec 30, 2020

h-vetinari reopened this Dec 30, 2020

facebook-github-bot added the CLA Signed label Dec 30, 2020

mdouze reviewed Jan 18, 2021

View reviewed changes

review mdouze: keep instruction_set()

6d3f5a7

h-vetinari force-pushed the master branch from 0a51a50 to 6d3f5a7 Compare January 18, 2021 11:52

facebook-github-bot reviewed Jan 21, 2021

View reviewed changes

remove redundant return statements

f2df3a4

facebook-github-bot reviewed Jan 29, 2021

View reviewed changes

mdouze reviewed Jan 29, 2021

View reviewed changes

avoid KeyError (review mdouze)

95d60df

h-vetinari force-pushed the master branch from 25ea945 to 95d60df Compare January 29, 2021 11:00

h-vetinari added 2 commits January 29, 2021 16:12

turn instruction_set into set (review beauby)

0d6863b

rename instruction_set to something less ambiguous

4bfa7bf

It is a set of instruction sets, and "instruction_set" is not great for that

h-vetinari force-pushed the master branch from 13289c4 to 4bfa7bf Compare January 29, 2021 15:13

facebook-github-bot reviewed Feb 3, 2021

View reviewed changes

facebook-github-bot closed this in 16b4e88 Feb 3, 2021

facebook-github-bot added the Merged label Feb 3, 2021

h-vetinari mentioned this pull request Feb 4, 2021

Build with AVX2 support conda-forge/faiss-split-feedstock#27

Merged

h-vetinari mentioned this pull request Feb 12, 2021

add msvc-compatible AVX2 switch in CMakeLists.txt #1680

Closed

This was referenced Feb 12, 2021

Win+AVX2 compat #1681

Closed

make setup.py win+avx2 compatible #1682

Closed

log success messages and errors in loader.py #1683

Closed

Windows: portable intrinsics #1684

Closed

matsui528 mentioned this pull request May 11, 2021

_swigfaiss_avx2.so may not be loaded properly in conda #1711

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make AVX2-detection platform-independent #1600

make AVX2-detection platform-independent #1600

h-vetinari commented Dec 30, 2020 •

edited

Loading

facebook-github-bot commented Dec 30, 2020

facebook-github-bot commented Dec 30, 2020

h-vetinari commented Jan 11, 2021

mdouze commented Jan 12, 2021

h-vetinari commented Jan 12, 2021

mdouze Jan 18, 2021

h-vetinari commented Jan 20, 2021

facebook-github-bot left a comment

facebook-github-bot left a comment

mdouze left a comment

mdouze Jan 29, 2021

h-vetinari Jan 29, 2021

beauby Jan 29, 2021

h-vetinari Jan 29, 2021

mdouze Feb 3, 2021

facebook-github-bot left a comment

facebook-github-bot commented Feb 3, 2021

make AVX2-detection platform-independent #1600

make AVX2-detection platform-independent #1600

Conversation

h-vetinari commented Dec 30, 2020 • edited Loading

facebook-github-bot commented Dec 30, 2020

facebook-github-bot commented Dec 30, 2020

h-vetinari commented Jan 11, 2021

mdouze commented Jan 12, 2021

h-vetinari commented Jan 12, 2021

mdouze Jan 18, 2021

Choose a reason for hiding this comment

h-vetinari commented Jan 20, 2021

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

mdouze left a comment

Choose a reason for hiding this comment

mdouze Jan 29, 2021

Choose a reason for hiding this comment

h-vetinari Jan 29, 2021

Choose a reason for hiding this comment

beauby Jan 29, 2021

Choose a reason for hiding this comment

h-vetinari Jan 29, 2021

Choose a reason for hiding this comment

mdouze Feb 3, 2021

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 3, 2021

h-vetinari commented Dec 30, 2020 •

edited

Loading