kakao / n2

TOROS N2 - lightweight approximate Nearest Neighbor library which runs fast even with large datasets
Apache License 2.0
569 stars 70 forks source link

[MacOS] Support clang #6

Closed corona10 closed 4 years ago

corona10 commented 6 years ago

Current, N2 highly depends on GCC and openmp. This is why we need to install GCC in MacOS environment to use N2.

We need to find a way to support clang

ummae commented 6 years ago

Well, installing gcc on MacOS does not seem like a big deal. It would be nice to support clang, but I don't think it's high priority now. PR is still welcome ;)

cynthia commented 6 years ago

Actually, you don't need gcc - the LLVM distribution you can get off of Homebrew (brew install llvm) supports OpenMP just fine.

Is OpenMP an absolute requirement? I can see use cases where single threads make more sense - which would suggest OpenMP is extra clutter that isn't needed being linked in.

cynthia commented 6 years ago

Building on clang as of current master seems to work with the following steps - I don't know what would be the best way to hoist it into the current build instructions as it is very GCC centric.

brew install llvm
export CXX=/usr/local/opt/llvm/bin/clang++
export LDFLAGS=-L/usr/local/opt/llvm/lib
make

To build on Xcode's retarded clang, you'll have to apply this patch (which I didn't add in the PR #18, as the build rules do not have platform level conditionals and I don't quite know how you plan to add them. It also changes the performance characteristics so you'll need to add extra bits into the benchmarks about single threading scenarios - and I have no idea how you made the charts so I'm not volunteering to send in a PR for this bit.

diff --git a/src/Makefile b/src/Makefile
index 7030243..db14c3a 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -1,5 +1,5 @@
 CXX ?= g++
-CXXFLAGS += -O3 -march=native -std=c++11 -pthread -fPIC -fopenmp
+CXXFLAGS += -O3 -march=native -std=c++11 -pthread -fPIC
 CXXFLAGS += -I../third_party/spdlog/include/ -I../include/
 LDFLAGS += -lpthread

If llvm clang is "good enough" this ticket can be closed, but otherwise you'll need to figure out how to make OpenMP optional. (or conditionally disabled with a sanity check against CXX.)

lancifollia commented 6 years ago

@cynthia Thank you for your suggestion. However, to make openmp optionable is not suitable for this library. Since multi-threading is a crucial part of build/search performance.

cynthia commented 6 years ago

Well, in that case I think you should probably consider this "fixed" - Apple LLVM isn't getting OpenMP in the foreseeable future. Compiling against non-Apple LLVM clang works fine with the patch above.

The only sane way to get this to build out of the box on macOS without installing a new toolchain is by making the OpenMP dependency optional. (Most likely there are some insane hacks that might work, at least until the next Xcode release. Probably not what you are looking for.)

gony-noreply commented 4 years ago

As in the comments above, N2 does not plan to support build with clang(installed with Xcode). This is because OpenMP is essential for N2 and it is not difficult to install a new toolchain like gcc or llvm.

In the recently released n2 including 0.1.7, n2 python setup script forced to use gcc installed with brew. This was for a more easy-to-use guide, but I think it was too compelling. So through a recent patch, we changed it to allow other compilers like llvm as long as the user set environment variables correctly. The patch was pushed in the current dev branch.

Below is a more general example script for building N2 using llvm installed with brew.

brew install llvm
export CC=$(brew --prefix llvm)/bin/clang
export CXX=$(brew --prefix llvm)/bin/clang++
export LDFLAGS=-L$(brew --prefix llvm)/lib
# `make` or `python setup.py install`

For macOS users, our guide is still to use gcc installed with brew, but users can install and configure a compiler like llvm if wish. In particular, If there are users who want to use clang, I think that llvm installed with brew is enough.