Skip to content
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

Windows: portable intrinsics #1684

Closed
wants to merge 4 commits into from

Conversation

h-vetinari
Copy link
Contributor

@h-vetinari h-vetinari commented Feb 12, 2021

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.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Feb 12, 2021

The failing test seems to be flaky.

@beauby beauby requested a review from mdouze February 12, 2021 14:56
@beauby
Copy link
Contributor

beauby commented Feb 12, 2021

Looking good, @mdouze could you take a look?

@mdouze
Copy link
Contributor

mdouze commented Feb 15, 2021

I will take a look. The contbuild system on Linux does not compile with AVX2 enabled...

@mdouze
Copy link
Contributor

mdouze commented Feb 15, 2021

Looks good to me.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@h-vetinari
Copy link
Contributor Author

@mdouze, would you prefer me to rebase this on master after #1690? Or anything else I can help with? I'm kinda waiting with conda-forge/faiss-split-feedstock#27 until this goes in because I don't want to get this patch wrong.

@facebook-github-bot
Copy link
Contributor

@mdouze merged this pull request in 1afaddb.

@h-vetinari h-vetinari deleted the win_intrinsics branch February 20, 2021 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants