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

Add C++ compatibility with macOS #474

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Conversation

darvg
Copy link
Contributor

@darvg darvg commented Oct 12, 2023

  • Does this PR have a descriptive title that could go in our release notes?
  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
    • Is the change to the API backwards compatible?
  • Should this result in any changes to our documentation, either updating existing docs or adding new ones?

Reference Issues/PRs

What does this implement/fix? Briefly explain your changes.

Addresses a number of discrepancies preventing DiskANN from building on macOS (Mac-only changes):

  1. no MKL -- replaced with Accelerate
  2. no libaio -- replaced with libdispatch
  3. uint64_t = unsigned long long on macOS, while uint64_t = unsigned long on Ubuntu and Windows: uint64_t and size_t are used interchangeably throughout the code because of the latter equivalence, which causes problems on macOS
  4. other small changes

Any other comments?

I'm not very familiar with the CMake syntax, so in particular, checking those changes would be appreciated.

@harsha-simhadri
Copy link
Contributor

@darvg can you please add some CI tests for mac to validate the changes.

@daxpryce daxpryce self-requested a review January 3, 2024 20:52
@daxpryce
Copy link
Contributor

daxpryce commented Jan 3, 2024

@darvg - I'm going to give this a pull and see how close this gets us to a python release for macos too. thank you for this contribution

@daxpryce
Copy link
Contributor

daxpryce commented Jan 25, 2024

I'm getting an error on build now:

MacOS version:

dax@cloudship DiskANN: $ sw_vers
ProductName:		macOS
ProductVersion:		14.3
BuildVersion:		23D56
dax@cloudship DiskANN: $ cmake --build build
[  0%] Building CXX object src/CMakeFiles/diskann.dir/abstract_data_store.cpp.o
[  1%] Building CXX object src/CMakeFiles/diskann.dir/ann_exception.cpp.o
[  2%] Building CXX object src/CMakeFiles/diskann.dir/apple_aligned_file_reader.cpp.o
In file included from /Users/dax/dev/DiskANN/src/apple_aligned_file_reader.cpp:1:
In file included from /Users/dax/dev/DiskANN/include/aligned_file_reader.h:84:
/Users/dax/dev/DiskANN/include/utils.h:39:9: fatal error: '_MM_HINT_T0' macro redefined [-Wmacro-redefined]
git pull darvg
git checkout sid/apple
cmake -S . -B build
brew install boost clang-format libomp gperftools
cmake -S . -B build
cmake --build build

Mine is an intel based macbook too, so I don't know if that has something to do with this or what. Imagine I've never built a cxx project on mac before, because that's exactly where I am at!

@darvg
Copy link
Contributor Author

darvg commented Jan 26, 2024

@daxpryce I believe this error is popping up because _MM_HINT_T0 is already defined on Intel computers for _mm_prefetch, but not on ARM computers. So the redefinition error would naturally appear on Intel computers. If you think it's necessary in this PR, I can look into adding Intel Mac compatibility as well to the PR.

@daxpryce daxpryce requested a review from rakri January 26, 2024 20:19
@daxpryce
Copy link
Contributor

@daxpryce I believe this error is popping up because _MM_HINT_T0 is already defined on Intel computers for _mm_prefetch, but not on ARM computers. So the redefinition error would naturally appear on Intel computers. If you think it's necessary in this PR, I can look into adding Intel Mac compatibility as well to the PR.

I don't necessarily think that this PR needs to address it, but we should create an issue to support x86_64 macOS as well in the future. I'm not really able to pull this and test it out locally so I'm unsure I can really do a good code review - at least not any better than @rakri would do :) I guess if it looks sane and builds on the CI action runners (note to everyone else: picking the macos-latest-xlarge is crucial for testing an aarch64 based instruction set here, whereas I believe macos-latest-large would still be x86_64 as per https://github.blog/2023-10-02-introducing-the-new-apple-silicon-powered-m1-macos-larger-runner-for-github-actions/ )

@darvg
Copy link
Contributor Author

darvg commented Jan 27, 2024

@daxpryce I added in a possible fix for compiling on Intel Machines (luckily, I think the only issue is the one you reported). The GitHub action didn't trigger, but I tried compiling it locally with Rosetta and it built, so hopefully there are no other errors.

@daxpryce
Copy link
Contributor

daxpryce commented Feb 6, 2024

@darvg I'm getting a new build error:

dax@cloudship DiskANN: $ cmake --build build
[  0%] Building CXX object src/CMakeFiles/diskann.dir/abstract_data_store.cpp.o
[  1%] Building CXX object src/CMakeFiles/diskann.dir/ann_exception.cpp.o
[  2%] Building CXX object src/CMakeFiles/diskann.dir/apple_aligned_file_reader.cpp.o
In file included from /Users/dax/dev/DiskANN/src/apple_aligned_file_reader.cpp:1:
In file included from /Users/dax/dev/DiskANN/include/aligned_file_reader.h:84:
/Users/dax/dev/DiskANN/include/utils.h:46:51: fatal error: a type specifier is required for all declarations
static inline __attribute__((always_inline)) void _mm_prefetch(char const *p, int i)
                                                  ^
/Library/Developer/CommandLineTools/usr/lib/clang/15.0.0/include/xmmintrin.h:2107:51: note: expanded from macro '_mm_prefetch'
#define _mm_prefetch(a, sel) (__builtin_prefetch((const void *)(a), \
                                                  ^
1 error generated.
make[2]: *** [src/CMakeFiles/diskann.dir/apple_aligned_file_reader.cpp.o] Error 1
make[1]: *** [src/CMakeFiles/diskann.dir/all] Error 2
make: *** [all] Error 2

I may not be reading this right, but is it saying that the xmmintrin header has added some inline code that is invalid with the version of g++ we're building with? Do we need to build with clang?

I know enough to get myself into trouble and chase a lot of red herrings, so I hope you have a better idea than I do :)

@darvg
Copy link
Contributor Author

darvg commented Feb 7, 2024

@daxpryce CMake should be using the built-in apple-clang for builds (setting up GCC or another version of LLVM is just too difficult). Regarding the error, I just pushed a possible fix.

@daxpryce
Copy link
Contributor

new errors, but it's looking closer:

/Users/dax/dev/DiskANN/include/utils.h:119:9: fatal error: 'sgesdd_' is deprecated: first deprecated in macOS 13.3 - The CLAPACK interface is deprecated.  Please compile with -DACCELERATE_NEW_LAPACK to access the new lapack headers. [-Wdeprecated-declarations]
        sgesdd_(&jobz, &m, &n, a, &lda_t, s, u, &ldu_t, vt, &ldvt_t, work, &lwork, iwork, &info);

I tried again after adding add_compile_definitions(ACCELERATE_NEW_LAPACK) after line 162 in the CMakeLists.txt, but then I got

In file included from /Users/dax/dev/DiskANN/include/pq.h:6:
/Users/dax/dev/DiskANN/include/utils.h:59:9: fatal error: unknown type name '__CLPK_integer'
typedef __CLPK_integer clp_int;
        ^
1 error generated.
make[2]: *** [src/CMakeFiles/diskann.dir/pq.cpp.o] Error 1

@darvg
Copy link
Contributor Author

darvg commented Feb 13, 2024

@daxpryce, could you please verify that the following file exists for you?
/Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk/System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/clapack.h

You might need to change the SDK version number depending on which one you're compiling against. There should be a definition for __CLPK_INTEGER near the top of that file.

@daxpryce
Copy link
Contributor

@darvg - apparently my version is MacOS13.3.sdk, and there definitely is a __CLPK_integer in it.

with line#'s from /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Accelerate.framework/Frameworks/vecLib.framework/Headers/clapack.h

Note that MacOSX.sdk is a symlink to MacOSX14.2.sdk

   33 #if defined(__LP64__) /* In LP64 match sizes with the 32 bit ABI */
   34     typedef int                 __CLPK_integer;
   35     typedef int                 __CLPK_logical;
   36     typedef float               __CLPK_real;
   37     typedef double              __CLPK_doublereal;
   38
   39 #pragma clang diagnostic push
   40 #pragma clang diagnostic ignored "-Wstrict-prototypes"
   41     typedef __CLPK_logical      (*__CLPK_L_fp)();
   42 #pragma clang diagnostic pop
   43
   44     typedef int                 __CLPK_ftnlen;
   45 #else
   46     typedef long int    __CLPK_integer;
   47     typedef long int    __CLPK_logical;
   48     typedef float               __CLPK_real;
   49     typedef double              __CLPK_doublereal;
   50
   51 #pragma clang diagnostic push
   52 #pragma clang diagnostic ignored "-Wstrict-prototypes"
   53     typedef __CLPK_logical      (*__CLPK_L_fp)();
   54 #pragma clang diagnostic pop
   55
   56     typedef long int    __CLPK_ftnlen;
   57 #endif

So there's very clearly a typedef for it. I checked the CMakeLists.txt to see if we added an include_directories for this header and we hadn't, but adding it around ~ 162 by doing include_directories("${ACCELERATE_LIBRARY}/Frameworks/vecLib.framework/Headers") didn't change anything. I feel like we're really close though!

@darvg
Copy link
Contributor Author

darvg commented Feb 14, 2024

@daxpryce let me try updating my mac (it's currently on macOS 12) to see if I can reproduce the issue. In the meantime, maybe try reinstalling your xcode developer tools (or command line tools) and see if that fixes the issue?

@daxpryce
Copy link
Contributor

@darvg I'm like 99% sure I got it fully reinstalled, but got the same error. I know a coworker with a macbook too, so I can probably get him to give his environment a try on Tuesday (msft has a US holiday on Monday). I'll report back with that.

If it does end up being this particular incarnation of my system, I have absolutely no qualms wiping my macbook disk and reinstalling. It's due, anyway. I'll keep you posted, and thanks for trying to work through this with me!

@darvg
Copy link
Contributor Author

darvg commented Feb 16, 2024

Sure, sounds good. I updated my Mac to macOS 14 and I still haven't been able to reproduce the error, so I'm not entirely sure what's going on. It doesn't look like an Intel/ARM issue to me. For now, if you want to get to benchmarking, you can try replacing the typedef in utils.h with a regular int (which it seems to be anyways in a transitive sense).

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