ggml : fix GGML_F32_VEC_FMA argument order in ggml_vec_mad1_f32

[CISC]

See #16063 (comment)

[ggerganov]

Don't think this is correct. The order for the GGML_F32_VEC_FMA macro is always the same, but it can differ on some instructions sets. Which one are you using?

I think this one is wrong:

llama.cpp/ggml/src/ggml-cpu/simd-mappings.h

Lines 821 to 824 in 6a2c614

	#if defined(__FMA__)
	// TODO: Does this work?
	#define GGML_F32x4_FMA(a, b, c) _mm_fmadd_ps(b, c, a)
	#else

https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_fmadd_ps&ig_expand=3103

[CISC]

Don't think this is correct. The order for the GGML_F32_VEC_FMA macro is always the same, but it can differ on some instructions sets. Which one are you using?

This one, but if I change that everything else breaks

llama.cpp/ggml/src/ggml-cpu/simd-mappings.h

Lines 533 to 537 in 6a2c614

	#if defined(__FMA__)
	#define GGML_F32x8_FMA(a, b, c) _mm256_fmadd_ps(b, c, a)
	#else
	#define GGML_F32x8_FMA(a, b, c) _mm256_add_ps(_mm256_mul_ps(b, c), a)
	#endif

[CISC]

This is confusing, some places it's used like this:

llama.cpp/ggml/src/ggml-cpu/vec.h

Line 300 in 05c0380

ay2 = GGML_F32_VEC_FMA(ay2, ax2, vx);

and other places like this:

llama.cpp/ggml/src/ggml-cpu/vec.h

Line 248 in 05c0380

sum[k][j] = GGML_F16_VEC_FMA(sum[k][j], ax[j], ay[j]);

[CISC]

According to Intel it should be a * b + c, so something is screwy:
https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm256_fmadd_ps&ig_expand=3107

[ggerganov]

It appears that the meaning of GGML_F32_VEC_FMA(a, b, c) and GGML_F16_VEC_FMA(a, b, c) is intended to be:

b*c + a

So the change that you propose makes sense now and this is inline with the Intel docs.

[CISC]

So the change that you propose makes sense now and this is inline with the Intel docs.

Yes, I think so too, it's just somewhat confusing with the order and naming between vec_mad1 and vec_mad functions...

[CISC]

It seems this codepath is not taken when built for CUDA as the backend-ops test succeeds...

Edit: I guess this means we never really test hardware optimized CPU backend?

[ggerganov]

It seems this codepath is not taken when built for CUDA as the backend-ops test succeeds...

Edit: I guess this means we never really test hardware optimized CPU backend?

I think the reason we don't see it is because the tests currently have ne0 == 10 which is too small to enter the SIMD path. If you add a larger tests (for example, ne0 == 100) it should fail.

llama.cpp/tests/test-backend-ops.cpp

Lines 6156 to 6157 in d9e0e7c

	test_cases.emplace_back(new test_scale());
	test_cases.emplace_back(new test_scale(GGML_TYPE_F32, {10, 10, 10, 10}, 2.0f, 1.0f));

[CISC]

It seems this codepath is not taken when built for CUDA as the backend-ops test succeeds...
Edit: I guess this means we never really test hardware optimized CPU backend?

I think the reason we don't see it is because the tests currently have ne0 == 10 which is too small to enter the SIMD path. If you add a larger tests (for example, ne0 == 100) it should fail.

Yep, it does, I'll add the test.