ggml : fix MIN / MAX macros

[ggerganov]

No description provided.

[wtarreau]

Hi Georgi,

This patch just made me notice that your MIN/MAX macros are defined without a preliminary copy so that one term is evaluated twice:

#define MIN(a, b) ((a) < (b) ? (a) : (b))
#define MAX(a, b) ((a) > (b) ? (a) : (b))

I had a quick look at the code and overall it looks safe (i.e. no "x++" there), though there are a few occurrences of function calls, which I think are not in fast paths.

This can be something newcomers will not necessarily notice and some might get trapped however. How about using the more robust compound expressions instead, typically something like this:

#define MIN(a, b) ({ typeof(a) __a = (a); typeof(b) __b = (b);  \
                     ((__a) < (__b) ? (__a) : (__b));           \
                  })

#define MAX(a, b) ({ typeof(a) __a = (a); typeof(b) __b = (b);  \
                     ((__a) > (__b) ? (__a) : (__b));           \
                  })

[ggerganov]

Yes, would you like to submit a PR?

[wtarreau]

If that really saves you time I'll do it, but just not right now. If it's painless enough for you to copy-paste these few lines and commit them yourself, that's way faster for me, no need to even credit me, I don't care. I'm terribly bad with github, every tiny operation in it takes me one hour. However I would definitely go through it for larger patches.

[ggerganov]

No worries - it's easier for me to push the change myself. Thanks

[slaren]

typeof may not be available on every compiler.

[ggerganov]

typeof may not be available on every compiler.

Yeah, with clang I get these errors:

$ ▶ LLAMA_NO_CCACHE=1 make
I llama.cpp build info: 
I UNAME_S:   Darwin
I UNAME_P:   arm
I UNAME_M:   arm64
I CFLAGS:    -I. -Icommon -D_XOPEN_SOURCE=600 -D_DARWIN_C_SOURCE -DNDEBUG -DGGML_USE_ACCELERATE -DACCELERATE_NEW_LAPACK -DACCELERATE_LAPACK_ILP64 -DGGML_USE_LLAMAFILE -DGGML_USE_METAL -DGGML_METAL_EMBED_LIBRARY  -std=c11   -fPIC -O3 -Wall -Wextra -Wpedantic -Wcast-qual -Wno-unused-function -Wshadow -Wstrict-prototypes -Wpointer-arith -Wmissing-prototypes -Werror=implicit-int -Werror=implicit-function-declaration -pthread -Wunreachable-code-break -Wunreachable-code-return -Wdouble-promotion 
I CXXFLAGS:  -std=c++11 -fPIC -O3 -Wall -Wextra -Wpedantic -Wcast-qual -Wno-unused-function -Wmissing-declarations -Wmissing-noreturn -pthread   -Wunreachable-code-break -Wunreachable-code-return -Wmissing-prototypes -Wextra-semi -I. -Icommon -D_XOPEN_SOURCE=600 -D_DARWIN_C_SOURCE -DNDEBUG -DGGML_USE_ACCELERATE -DACCELERATE_NEW_LAPACK -DACCELERATE_LAPACK_ILP64 -DGGML_USE_LLAMAFILE -DGGML_USE_METAL -DGGML_METAL_EMBED_LIBRARY 
I NVCCFLAGS: -std=c++11 -O3 
I LDFLAGS:   -framework Accelerate -framework Foundation -framework Metal -framework MetalKit 
I CC:        Apple clang version 15.0.0 (clang-1500.1.0.2.5)
I CXX:       Apple clang version 15.0.0 (clang-1500.1.0.2.5)

cc -I. -Icommon -D_XOPEN_SOURCE=600 -D_DARWIN_C_SOURCE -DNDEBUG -DGGML_USE_ACCELERATE -DACCELERATE_NEW_LAPACK -DACCELERATE_LAPACK_ILP64 -DGGML_USE_LLAMAFILE -DGGML_USE_METAL -DGGML_METAL_EMBED_LIBRARY  -std=c11   -fPIC -O3 -Wall -Wextra -Wpedantic -Wcast-qual -Wno-unused-function -Wshadow -Wstrict-prototypes -Wpointer-arith -Wmissing-prototypes -Werror=implicit-int -Werror=implicit-function-declaration -pthread -Wunreachable-code-break -Wunreachable-code-return -Wdouble-promotion     -c ggml-quants.c -o ggml-quants.o
ggml-quants.c:282:33: warning: use of GNU statement expression extension from macro expansion [-Wgnu-statement-expression-from-macro-expansion]
            const uint8_t xi0 = MIN(15, (int8_t)(x0 + 8.5f));
                                ^
./ggml-impl.h:17:20: note: expanded from macro 'MIN'
#define MIN(a, b) ({ typeof(a) __a = (a); typeof(b) __b = (b);  \
                   ^
ggml-quants.c:282:33: error: call to undeclared function 'typeof'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
./ggml-impl.h:17:22: note: expanded from macro 'MIN'
#define MIN(a, b) ({ typeof(a) __a = (a); typeof(b) __b = (b);  \

So looks like it's not very portable unfortunately as proposed

[wtarreau]

@slaren which ones do you have in mind ? I'm using this on fairly old gcc 3.4+ for example, but admittedly I'm not using compilers found on non-unix systems.

[slaren]

From what I can tell, typeof was just added to C23. Before that, it seems to be a gcc extension.

[wtarreau]

Ah it didn't take long :-( Stange, for me clang-13 on linux supports it.

[wtarreau]

So just for the record, for me on my local tests it's OK from gcc-3.4 to gcc-13.2, and on clang-3.8 and clang-13, both C and C++. It's possible that it's blocked by one of the build options (and I understand that you may not want to adjust them for portability reasons, e.g. if it's not available on windows for example). There are probably other approaches such as using "auto" on recent compilers, not sure if these would work here.

[wtarreau]

OK, bad idea, "type defaults to int" says gcc-13.2. Let's leave this at rest for now, I really need to finish other stuff. Sorry for not having better to propose for now.

[ggerganov]

No problem - at the very least I will be more careful what I put in the MIN/MAX arguments from now on :)