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

didzis · 2024-04-12T11:43:18Z

Adds missing AVX512 detection to Makefile and CMakeLists.txt

przemoc · 2024-04-12T13:51:06Z

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). :)

didzis · 2024-04-12T15:18:51Z

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.

przemoc · 2024-04-12T15:16:33Z

CMakeLists.txt

-option(WHISPER_NO_F16C                "whisper: disable F16c" OFF)
+option(WHISPER_NO_AVX                 "whisper: disable AVX"    OFF)
+option(WHISPER_NO_AVX2                "whisper: disable AVX2"   OFF)
+option(WHISPER_AVX512                 "whisper: enable AVX512"  OFF)


I suggest using WHISPER_NO_AVX512 to be consistent with rest of these settings (until they will be overhauled one day in whisper.cpp to use positive notion instead of negative one), and naturally changing default to ON in such case.

Sorry if my rant in side note made you misunderstand the expectation for this PR.

Are you suggesting to use WHISPER_NO_AVX512 set to ON by default?

Yes, to be consistent, because all other ISA extension options in whisper.cpp have such polarity.
_{(Maybe we'll change it in the future, but if it will happen, then it should be done separately.)}

przemoc · 2024-04-12T15:23:44Z

Makefile

+		AVX512F_M := $(shell $(CPUINFO_CMD) | grep -iw 'AVX512F')
+		ifneq (,$(AVX512F_M))
+			CFLAGS   += -mavx512f
+			CXXFLAGS += -mavx512f
+		endif
+
+		AVX512DQ_M := $(shell $(CPUINFO_CMD) | grep -iw 'AVX512DQ')
+		ifneq (,$(AVX512DQ_M))
+			CFLAGS   += -mavx512dq
+			CXXFLAGS += -mavx512dq
+		endif
+
+		AVX512CD_M := $(shell $(CPUINFO_CMD) | grep -iw 'AVX512CD')
+		ifneq (,$(AVX512CD_M))
+			CFLAGS   += -mavx512cd
+			CXXFLAGS += -mavx512cd
+		endif
+
+		AVX512CD_M := $(shell $(CPUINFO_CMD) | grep -iw 'AVX512BW')
+		ifneq (,$(AVX512BW_M))
+			CFLAGS   += -mavx512bw
+			CXXFLAGS += -mavx512bw
+		endif
+
+		AVX512VL_M := $(shell $(CPUINFO_CMD) | grep -iw 'AVX512VL')
+		ifneq (,$(AVX512VL_M))
+			CFLAGS   += -mavx512vl
+			CXXFLAGS += -mavx512vl
+		endif


I suggest combining them into one block, where we check just for AVX512F, but then add flags related to all 5 AVX-512 subsets (F, CD, VL, DQ, BW). I don't believe Xeon Phi existence is good enough reason check all of these subsets individually. I never suggested checking them individually.

Table at https://en.wikipedia.org/wiki/AVX-512#CPUs_with_AVX-512 explains why.

Let's have a simpler code. If there will be mainstream CPU in the future not fitting into the assumption (F presence means presence of CD, VL, DQ and BW too), then we can start playing again with checking those flags individually.

przemoc · 2024-04-12T15:44:25Z

Makefile

+			CXXFLAGS += -mavx512vl
+		endif
+
+		AVX512VNNI_M := $(shell $(CPUINFO_CMD) | grep -iwE 'AVX512_VNNI|AVX512VNNI')


AVX512_VNNI is for Linux, so just out of curiosity:
Which OS uses AVX512VNNI?

As you're adding VNNI beside basic AVX-512 subsets, I would suggest adding also VBMI, as llama.cpp does it too. It's nice to have some kind of parity between these projects.

Moreover, if you're adding VNNI and VBMI in Makefile, you should add them in CMakeLists.txt too, via WHISPER_NO_AVX512_VNNI and WHISPER_NO_AVX512_VBMI, defaulting to ON. I suggest using add_compile_definitions similarly like llama.cpp does.

Added AVX512VNNI for pure symmetry just in case any system does have it.

I asked about AVX512VNNI, didn't request removing. But I see you removed it in dc69460.

So I looked around and it seems AVX512VNNI is the name reported by FreeBSD.
http://fxr.watson.org/fxr/source/x86/x86/identcpu.c#L994

Please bring back grep -iwE 'AVX512_VNNI|AVX512VNNI' for VNNI detection.

Great, my feeling was right then.

… VBMI

przemoc · 2024-04-12T18:20:11Z

CMakeLists.txt

+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /arch:AVX512")
+            set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /arch:AVX512")
+            set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /arch:AVX512")
+            # from llama.cpp:


I woud suggest removing from llama.cpp comment line. There are many things shared between llama.cpp and whisper.cpp, in both directions, so if we would be marking all of them, it would be pure noise. :)

Sure, just wanted to note, that I'm not author of that comment.

przemoc · 2024-04-12T18:29:58Z

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.)

didzis · 2024-04-12T18:55:51Z

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

przemoc · 2024-04-13T13:05:58Z

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)

ggerganov

@didzis Thank you for the contribution and @przemoc thank you for the thorough review!

…nov#2043) * make : add AVX512 detection to Makefile and CMakeLists.txt * make : autodetect more AVX512 instruction subsets * cmake : do not default to AVX512, must be enabled explicitly * cmake : enable a set of AVX512 subsets, when AVX512 is turned on * make : consolidate AVX512 subsets, add AVX512 VBMI * cmake : revert to NO AVX512 setting, add settings for AVX512 VNNI and VBMI * make : re-introduce AVX512VNNI back * cmake : remove superfluous comment line

make : add AVX512 detection to Makefile and CMakeLists.txt

c3753f9

didzis added 3 commits April 12, 2024 18:05

make : autodetect more AVX512 instruction subsets

e8ee795

cmake : do not default to AVX512, must be enabled explicitly

eb24a77

cmake : enable a set of AVX512 subsets, when AVX512 is turned on

ef25a4a

przemoc reviewed Apr 12, 2024

View reviewed changes

didzis added 2 commits April 12, 2024 20:20

make : consolidate AVX512 subsets, add AVX512 VBMI

dc69460

cmake : revert to NO AVX512 setting, add settings for AVX512 VNNI and…

f7c4acb

… VBMI

przemoc reviewed Apr 12, 2024

View reviewed changes

didzis added 2 commits April 12, 2024 21:45

make : re-introduce AVX512VNNI back

83c902b

cmake : remove superfluous comment line

9b0b1d1

przemoc approved these changes Apr 12, 2024

View reviewed changes

didzis changed the title ~~Add AVX512 detection to Makefile and CMakeLists.txt~~ build : detect AVX512 in Makefile, add AVX512 option in CMake Apr 13, 2024

ggerganov approved these changes Apr 15, 2024

View reviewed changes

ggerganov merged commit c7f95b7 into ggerganov:master Apr 15, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

didzis commented Apr 12, 2024

przemoc commented Apr 12, 2024 •

edited

Loading

didzis commented Apr 12, 2024

przemoc Apr 12, 2024

didzis Apr 12, 2024

przemoc Apr 12, 2024

przemoc Apr 12, 2024

przemoc Apr 12, 2024

didzis Apr 12, 2024

przemoc Apr 12, 2024

didzis Apr 12, 2024

przemoc Apr 12, 2024

didzis Apr 12, 2024

przemoc commented Apr 12, 2024

didzis commented Apr 12, 2024

przemoc commented Apr 13, 2024

ggerganov left a comment

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

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

Conversation

didzis commented Apr 12, 2024

przemoc commented Apr 12, 2024 • edited Loading

Issue 1 - AVX-512 as a default

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

didzis commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemoc commented Apr 12, 2024

didzis commented Apr 12, 2024

przemoc commented Apr 13, 2024

ggerganov left a comment

Choose a reason for hiding this comment

przemoc commented Apr 12, 2024 •

edited

Loading