whisper.cpp icon indicating copy to clipboard operation
whisper.cpp copied to clipboard

build : detect AVX512 in Makefile, add AVX512 option in CMake

Open didzis opened this issue 1 year ago • 5 comments
trafficstars

Adds missing AVX512 detection to Makefile and CMakeLists.txt

didzis avatar Apr 12 '24 11:04 didzis

Thank you for your contribution. I see a few issues in this PR as seen in c3753f9.

Issue 1 - AVX-512 as a default

For CMake PR defaults to build with AVX-512, which is not ubiquitous. Only recent generations of Intel CPUs and very recent generations of AMD CPUs support them.

That's why I don't think it's reasonable to target AVX-512 and force a lot of folks setting WHISPER_NO_AVX512 now.

Of course situation may change in next 5 years.

Side note: I dislike that we have negative way of expressing x86 ISA extensions. Maybe we should finally change it in whisper.cpp, but this PR most likely wouldn't be the right venue to do that, as such change should be isolated from adding additional stuff.

Issue 2 - Different AVX-512 enablement for MSVC vs rest

PR enables AVX things differently for CMake and make. AVX-512 consists of many instruction subsets, and not all of them have to be supported by CPU. For MSVC CMake it uses /arch:AVX512 which per https://learn.microsoft.com/en-us/cpp/build/reference/arch-x64.

The __AVX512F__, __AVX512CD__, __AVX512BW__, __AVX512DQ__ and __AVX512VL__ preprocessor symbols are defined when the /arch:AVX512 compiler option is specified. For more information, see Predefined macros.

For non-MSVC CMake and make it uses -mavx512f which enables AVX-512 Foundation (F) only.

From compatibility perspective:

  • AVX-512 Foundation (F)
  • AVX-512 Conflict Detection Instructions (CD)
  • AVX-512 Vector Length Extensions (VL)
  • AVX-512 Doubleword and Quadword Instructions (DQ)
  • AVX-512 Byte and Word Instructions (BW)

set is the safest to go with assuming some form of AVX-512 is supported (and that's why MSVC does what it does).

Side note: Xeon Phi is the exception (not supporting VL, DQ, BW), but per https://en.wikipedia.org/wiki/Xeon_Phi:
"In 2017, Intel announced that Knights Hill had been canceled"
"Intel announced they were discontinuing Knights Landing in summer 2018"
so there is no point in cluttering solution space with dead product, and those very few who have access to it will definitely know how to tweak CMake or make build process to achieve what they want.

That's why I think for non-MSVC CMake and make we should ideally target the same set.

For compiler it should be: -mavx512f -mavx512cd -mavx512vl -mavx512dq -mavx512bw

Note: For precise detection on Linux all following flags should be present avx512f avx512cd avx512vl avx512dq avx512bw For precise detection on other systems I cannot comment right now, would need to research, but I won't do that, because Detecting using avx512f alone seems good enough (at least on Linux, but I suspect same flag name on other OSes). :)

przemoc avatar Apr 12 '24 13:04 przemoc

Updated PR so that for CMake AVX512 must be enabled explicitly. Also added AVX512 subsets for this case as suggested, however I would argue that this will lead to "bad instruction" error during execution in case a particular subset is used by the code, but not supported by the target CPU. Although in practice this may not happen because it looks like the inference code does not use any other AVX512 instruction subsets than AVX512F. Updated Makefile to detect AVX512 subsets.

didzis avatar Apr 12 '24 15:04 didzis

So fixing VNNI detection for non-Linux and removing superfluous comment are the only remaining things. Rest looks good to me. (But I haven't tested locally building and running whisper.cpp w/ AVX512 enabled.)

przemoc avatar Apr 12 '24 18:04 przemoc

Tested inference on a Linux machine with AVX512 all five instruction subsets and AVX512 VNNI.

didzis avatar Apr 12 '24 18:04 didzis

One little thing. I would suggest changing PR title for two reasons:

  • it's partially wrong, because there is no x86 ISA extensions detection in CMake,
  • it doesn't follow style used in this project.

I would suggest following one:

build : support enabling use of AVX-512 (detected in make, via WHISPER_NO_AVX512=0 for CMake)

przemoc avatar Apr 13 '24 13:04 przemoc