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

Fix windows CI #2889

Closed
wants to merge 2 commits into from
Closed

Conversation

wx257osn2
Copy link
Contributor

@wx257osn2 wx257osn2 commented May 31, 2023

#2882 added a for loop, which has unsigned index, qualified with #pragma omp parallel for, but it seems that MSVC doesn't support unsigned index with #pragma omp parallel for (I think this would not be conformed to OpenMP specification, but...)

I (finally) change the loop with signed index. This changes introduce the precondition n <= std::numeric_limits<std::make_signed_t<std::size_t>>::max() , but usually this is true I think, so I just put this limitation as a comment instead of any FAISS_ASSERT or something like that.

MSVC doesn't accept unsigned index with pragma omp parallel for
OpenMP (at least the specification) doesn't accept the expression includes a cast as loop condition with pragma omp parallel for
@alexanderguzhva
Copy link
Contributor

@wx257osn2 , thanks for your commit. Which MSVC version is this?

@wx257osn2
Copy link
Contributor Author

wx257osn2 commented May 31, 2023

@alexanderguzhva According to build log of CI,

** Visual Studio 2019 Developer Command Prompt v16.11.18

-- The CXX compiler identification is MSVC 19.16.27048.0

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@alexanderguzhva merged this pull request in bbc95b1.

Thejas-bhat pushed a commit to blevesearch/faiss that referenced this pull request Sep 27, 2023
Summary:
facebookresearch#2882 added [a for loop, which has unsigned index, qualified with `#pragma omp parallel for`](https://github.com/facebookresearch/faiss/pull/2882/files#diff-5a89dcb99a1cce3f297c7f7dfc8e221306b281d4ced6dac1e0fc0fa54188195fR449-R452), but it seems that [MSVC doesn't support unsigned index with `#pragma omp parallel for`](https://app.circleci.com/pipelines/github/facebookresearch/faiss/4220/workflows/ee72de05-6ead-42d9-8ec5-44772e9fd41b/jobs/22529?invite=true#step-104-333) (I think this would not be conformed to OpenMP specification, but...)

I (finally) change the loop with signed index. This changes introduce the precondition `n <= std::numeric_limits<std::make_signed_t<std::size_t>>::max()` , but usually this is `true` I think, so I just put this limitation as a comment instead of any `FAISS_ASSERT` or something like that.

Pull Request resolved: facebookresearch#2889

Reviewed By: wickedfoo

Differential Revision: D46325322

Pulled By: alexanderguzhva

fbshipit-source-id: c68f4c8be3db188ac067e053c6c716e2896f75c0
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.

None yet

3 participants