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

Check before removing march=native #476

Closed
wants to merge 1 commit into from

Conversation

xhochy
Copy link

@xhochy xhochy commented Jun 23, 2023

If you set HNSWLIB_NO_NATIVE and build on Apple M1 the removal of march=native fails because it isn't in the list of arguments.

@yurymalkov yurymalkov changed the base branch from master to develop July 4, 2023 23:45
@yurymalkov
Copy link
Member

Hi @xhochy,
Can you check if #461 resolved the issue? It is merged to develop branch

@xhochy
Copy link
Author

xhochy commented Jul 5, 2023

From my understanding this is even worse as it removes the option overall to omit -march=native from the build.

@dyashuni
Copy link
Contributor

@xhochy
#461 should fix the issues with -march=native. It automatically checks if the flag is available on the platform. If the flag is not available it is removed. No need to set HNSWLIB_NO_NATIVE for macOS.
Do you think that we still need HNSWLIB_NO_NATIVE to manually control -march=native on macOS ?

@xhochy
Copy link
Author

xhochy commented Jul 10, 2023

We need a general switch to disable march=native as otherwise the binaries are no longer redistributable. The linked PR removes that possibility completely. The PR here only fixed this option for the corner case of Apple Silicon.

@yurymalkov
Copy link
Member

We need a general switch to disable march=native as otherwise the binaries are no longer redistributable

@xhochy Can you provide more details about this problem?
Context: at some point we implemented runtime checks for AVX and AVX512 (b801080), so for x64 I expected it to run on different (older) machines without problems.

@xhochy
Copy link
Author

xhochy commented Jul 11, 2023

These runtime checks are useful for code where you explicitly use AVX & co. The problem with march=native is that it instructs the compiler to use all instructions that are available on the build machine anywhere in the executable. Thus, yiu cannot control the usage of these instruction sets at runtime.

In the case of conda-forge, we build packages on Azure Pipelines where normally all the latest instruction sets are available but the users of the packages have older CPUs and not necessarily access to the latest AVX512 features. Thus binaries compiled with march=native may fail at arbitrary points in the execution.

@dyashuni
Copy link
Contributor

dyashuni commented Jul 12, 2023

@xhochy thank you, it makes sense
Could you please change the setup.py file from the develop branch to add back the environment variable HNSWLIB_NO_NATIVE to skip -march=native and -mcpu=apple-m1 ?

I think we can add
this

hnswlib/setup.py

Lines 81 to 82 in 6f2c82d

if not os.environ.get("HNSWLIB_NO_NATIVE"):
c_opts['unix'].append('-march=native')

and a check if not os.environ.get("HNSWLIB_NO_NATIVE"): here
https://github.com/nmslib/hnswlib/blob/develop/setup.py#L100-L114

@xhochy xhochy closed this Aug 23, 2023
@xhochy xhochy deleted the march-duplicate-removal branch August 23, 2023 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants