-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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 detection code for avx/avx2/etc #809
Conversation
@sw can you review this and merge? |
This seems to work fine here on Linux, but it's not really needed here as we have
A disadvantage to this PR is if the build output is not intended to be run on the build host, e.g. your build host doesn't have AVX512 but you want to build with it enabled in order to run it on another machine. This would prevent that. As far as I understand it, the failed checks I'm seeing are due to it trying the MSVC flags on GCC. I guess you could separate the tests by compiler, but that would make it more complicated. |
Maybe a better solution would be to have a LLAMA_PORTABLE option in cmake. If you set PORTABLE=OFF:
If you set PORTABLE=ON, you'll have to specify the options you want (LLAMA_AVX2 etc.) (Instead of LLAMA_PORTABLE, we might also have something like LLAMA_NATIVE with opposite meaning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to @sw comments:
Move these in a separate CMake script in a cmake
folder to reduce clutter in the main CMakeLists.txt file.
See whisper.cpp
for example: https://github.com/ggerganov/whisper.cpp/tree/master/cmake
@sw , I can confirm this works on Windows 10 |
I decided to do the following:
|
|
works for linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge this with LLAMA_NATIVE=OFF
by default + fix CMake style to use lowercase as everywhere else. I.e:
IF(LLAMA_NATIVE)
include(cmake/FindSIMD.cmake)
ENDIF()
becomes
if (LLAMA_NATIVE)
include(cmake/FindSIMD.cmake)
endif()
@howard0su bump |
* Add detection code for avx * Only check hardware when option is ON * Modify per code review sugguestions * Build locally will detect CPU * Fixes CMake style to use lowercase like everywhere else * cleanup * fix merge * linux/gcc version for testing * msvc combines avx2 and fma into /arch:AVX2 so check for both * cleanup * msvc only version * style * Update FindSIMD.cmake --------- Co-authored-by: Howard Su <howard0su@gmail.com> Co-authored-by: Jeremy Dunn <jeremydunn123@gmail.com>
…v#3923) * Add detection code for avx * Only check hardware when option is ON * Modify per code review sugguestions * Build locally will detect CPU * Fixes CMake style to use lowercase like everywhere else * cleanup * fix merge * linux/gcc version for testing * msvc combines avx2 and fma into /arch:AVX2 so check for both * cleanup * msvc only version * style * Update FindSIMD.cmake --------- Co-authored-by: Howard Su <howard0su@gmail.com> Co-authored-by: Jeremy Dunn <jeremydunn123@gmail.com>
Yes please. With the wrong CPU I just get: |
Add the code to check the build host to determine the right CPU feature.
This is convenient when build Windows version on the machine without AVX2.