[patch] Makefile: make cpu-feature-recognition more flexible under FreeBSD

[abelbabel]

Hi,

I have a small patch for review and possibly integration into master ...

Regards
abelbabel

diff --git a/Makefile b/Makefile
index a263101..278b037 100644
--- a/Makefile
+++ b/Makefile
@@ -57,7 +57,7 @@ endif
 
 # OS specific
 # TODO: support Windows
-ifeq ($(filter $(UNAME_S),Linux Darwin DragonFly FreeBSD NetBSD OpenBSD Haiku),$(UNAME_S))
+ifeq ($(UNAME_S),$(filter $(UNAME_S),Linux Darwin DragonFly FreeBSD NetBSD OpenBSD Haiku))
 	CFLAGS   += -pthread
 	CXXFLAGS += -pthread
 endif
@@ -65,9 +65,11 @@ endif
 # Architecture specific
 # TODO: probably these flags need to be tweaked on some architectures
 #       feel free to update the Makefile for your architecture and send a pull request or issue
-ifeq ($(UNAME_M),$(filter $(UNAME_M),x86_64 i686))
+ifeq ($(UNAME_M),$(filter $(UNAME_M),x86_64 i686 amd64))
 	ifeq ($(UNAME_S),Darwin)
 		CPUINFO_CMD := sysctl machdep.cpu.features
+	else ifeq ($(UNAME_S),FreeBSD)
+		CPUINFO_CMD := grep Features /var/run/dmesg.boot | tr ',' ' ' | tr '[[:upper:]]' '[[:lower:]]'
 	else ifeq ($(UNAME_S),Linux)
 		CPUINFO_CMD := cat /proc/cpuinfo
 	else ifneq (,$(filter MINGW32_NT% MINGW64_NT%,$(UNAME_S)))
@@ -76,8 +78,8 @@ ifeq ($(UNAME_M),$(filter $(UNAME_M),x86_64 i686))
 		CPUINFO_CMD := sysinfo -cpu
 	endif
 
-	ifdef CPUINFO_CMD  	
-    AVX_M := $(shell $(CPUINFO_CMD) | grep -m 1 "avx ")
+	ifdef CPUINFO_CMD
+		AVX_M := $(shell $(CPUINFO_CMD) | grep -m 1 "avx ")
 		ifneq (,$(findstring avx,$(AVX_M)))
 			CFLAGS += -mavx
 		endif
@@ -113,11 +115,8 @@ ifeq ($(UNAME_M),$(filter $(UNAME_M),x86_64 i686))
 		endif
 	endif
 endif
-ifeq ($(UNAME_M),amd64)
-	CFLAGS += -mavx -mavx2 -mfma -mf16c
-endif
 
-ifneq ($(filter ppc64%,$(UNAME_M)),)
+ifneq (,$(filter ppc64%,$(UNAME_M)))
 	POWER9_M := $(shell grep "POWER9" /proc/cpuinfo)
 	ifneq (,$(findstring POWER9,$(POWER9_M)))
 		CFLAGS += -mpower9-vector

[abelbabel]

By the way: Is there a reason why flags are only adapted for c-compiler, but not c++-compiler?

[abelbabel]

Patch could break flag-addition on other BSD-variants (DragonFly, NetBSD, OpenBSD) - I don't know if they relied on the ifeq ($(UNAME_M),amd64) before and furthermore I don't know how CPU-features can be determined on these variants? If it's the same like under FreeBSD, then else ifeq ($(UNAME_S),FreeBSD)-line needs to be completed with the other variants ...

[przemoc]

Thank you for your suggested changes. If you have changes already written, creating Pull Request is generally a better way to share them with project than creating Issue.

You sprinkled some Makefile changes (changing argument order, fixing indentation) other than the main proposed one (adding CPUINFO_CMD for FreeBSD), and such changes are typically better put in separate commits for readability, reviewability and revertability, even if they would be squashed into one commit when merging in the end (which is current maintainer's approach).

Unfortunately CPU features in /var/run/dmesg.boot (which is a snapshot of system message buffer from boot time) are reported only on DragonFlyBSD and FreeBSD.

When it comes to uname -m on BSDs on x86-64 aka x64 machines, it returns amd64 on FreeBSD, NetBSD, OpenBSD, but x86_64 on DragonFlyBSD.

As to why only CFLAGS are updated and not CXXFLAGS, I guess it's because optimizations are predominantly useful for ggml.c written in C, which does all the heavy lifting in the project, while rest, written in C or C++, is essentially wrapping its usage, so there is not much to gain there. But yeah, we can update both, and I was actually thinking about it some time ago, but I don't like sneaking side changes along with actual changes, and as explained earlier, gain would be negligible, so I didn't bother doing that yet when suggesting various building related improvements via my PRs.

I wrote in comment to #1212 that I plan to introduce simple application that would do CPUID stuff and return what's supported, so it could be used in Makefile on BSDs (or other OSes) that do not have other out-of-the-box means to check it. I just haven't got to implementing it (and ideally I want to do it in reusable way, so it could be used in other projects if desired).

But maybe before that will happen, we could indeed go with your suggestion to use /var/run/dmesg.boot to infer host CPU capabilities on FreeBSD (and DragonFlyBSD). I may prepare PR based on your suggestion, and do some other improvements along the way, because it seems that CPU feature detection might have been broken on Haiku with commit c5f9acf. I guess I may have to set up yet another VM next to my existing 4 BSD ones. ;)

[przemoc]

Created PR in #1238.

[przemoc]

#1238 got merged yesterday, so this issue (#1234) can possibly be closed now.