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 detection code for avx/avx2/etc #809

Closed
wants to merge 4 commits into from

Conversation

howard0su
Copy link
Collaborator

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.

@howard0su
Copy link
Collaborator Author

@sw can you review this and merge?

@sw
Copy link
Collaborator

sw commented Apr 10, 2023

This seems to work fine here on Linux, but it's not really needed here as we have -march=native. I don't have Windows to test, so someone else should probably have a look.

CHECK_SSE should better be called CHECK_SIMD, as SSE is a specific instruction set, the predecessor to AVX.

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.

@sw
Copy link
Collaborator

sw commented Apr 10, 2023

Maybe a better solution would be to have a LLAMA_PORTABLE option in cmake.

If you set PORTABLE=OFF:

  • on gcc it will set -march=native
  • on MSVC it will run your checks, but just for the MSVC flags, and enable all that succeed

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)

Copy link
Owner

@ggerganov ggerganov left a 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

@arikpoz
Copy link
Contributor

arikpoz commented Apr 14, 2023

@sw , I can confirm this works on Windows 10
I debugged a crash in quantize that was due to AVX not supported on Intel Pentium G3220 CPU. I thought of fixing the AVX flag in CMakeLists.txt, and decided to look for existing issues / PRs first. I've found this one and can confirm it works well to detect the missing AVX flag, and that quantize is now working properly

@howard0su
Copy link
Collaborator Author

I decided to do the following:

  1. only do the check while LLAMA_NATIVE and MSVC both are TRUE.
  2. Leave LLAMA_NATIVE as default OFF to keep the current behavior. However I think it is better to ON as most users are building for its own box.
  3. Didn't introduce _PORTABLE as NATIVE is an existing variable. keep it simple.

@diwu1989
Copy link

only do the check while LLAMA_NATIVE and MSVC both are TRUE.
Does this mean that this fix is only for Windows? Can you make it also work on Linux?

@howard0su
Copy link
Collaborator Author

works for linux

Copy link
Owner

@ggerganov ggerganov left a 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()

@TFWol
Copy link

TFWol commented Jul 21, 2023

@howard0su bump

ggerganov pushed a commit that referenced this pull request Nov 5, 2023
* 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>
olexiyb pushed a commit to Sanctum-AI/llama.cpp that referenced this pull request Nov 23, 2023
…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>
@melroy89
Copy link

Yes please. With the wrong CPU I just get: signal: illegal instruction (core dumped).. Crash!

@ggerganov ggerganov closed this Feb 19, 2024
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.

None yet

7 participants