Don't use AVX2 instructions if the CPU don't support it #2571

TedLyngmo · 2025-09-02T08:57:40Z

bitops.c: serverPopcount() used popcountAVX2(), which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error.

Added a __builtin_cpu_supports("avx2") check and falling back to the platform agnostic version if AVX2 is not supported.

Fixes #2570

codecov · 2025-09-02T09:16:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.22%. Comparing base (eebed88) to head (2b9e8e3).
⚠️ Report is 4 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2571      +/-   ##
============================================
+ Coverage     72.20%   72.22%   +0.02%     
============================================
  Files           126      126              
  Lines         70660    70660              
============================================
+ Hits          51017    51034      +17     
+ Misses        19643    19626      -17

Files with missing lines	Coverage Δ
src/bitops.c	`94.08% <100.00%> (ø)`

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ranshid · 2025-09-02T15:37:59Z

Trying to bring back my memory regarding SSE2 support. I am not sure SSE2 (which is something we use under the HAVE_X86_SIMD) require AVX2 support. Also per the bug you mentioned I think it will not really solve the problem in case we are using an AVX2 cross compiled binary on cpu which has no AVX2 support. I think a better fix would be to do the same thing we did for hyperloglog in #1293 where we make a runtime check on the CPU property supports avx2.

TedLyngmo · 2025-09-02T15:51:31Z

Trying to bring back my memory regarding SSE2 support. I am not sure SSE2 (which is something we use under the HAVE_X86_SIMD) require AVX2 support.

I don't know. I only know that I got the illegal instruction error and that the server's CPU supports AVX but not AVX2. I found these intrinsic functions used in the commit that introduced the error:

 _mm256_add_epi64
 _mm256_add_epi8
 _mm256_and_si256
 _mm256_extract_epi64
 _mm256_loadu_si256
 _mm256_set1_epi8
 _mm256_setr_epi8
 _mm256_setzero_si256
 _mm256_shuffle_epi8
 _mm256_srli_epi16

Also per the bug you mentioned I think it will not really solve the problem in case we are using an AVX2 cross compiled binary on cpu which has no AVX2 support. I think a better fix would be to do the same thing we did for hyperloglog in #1293 where we make a runtime check on the CPU property supports avx2.

Sure, whatever works :-) Too bad everyone will have to suffer a runtime check every time these functions are going to be used though. It'll bloat the binary a bit too.

Btw: What cross compiler does not define __AVX2__ if the target supports it? That sounds like a flaw that should be reported.

zuiderkwast · 2025-09-02T16:11:02Z

Well spotted. #1741 was released in 8.1 so we'll need to backport this fix to 8.1.

Please use a more descriptive text in the PRs, rather than only referring to an issue. When we merge, we squash-merge all the commits and use the PR title and description as the commit message. (We don't always need an issue btw. Only a PR is fine.)

Sure, whatever works :-) Too bad everyone will have to suffer a runtime check every time these functions are going to be used though. It'll bloat the binary a bit too.

Btw: What cross compiler does not define __AVX2__ if the target supports it? That sounds like a flaw that should be reported.

The same amd64 binary can be used by all amd64 machines, with or without various AVX support. Binary files are often distributed in distros and in containers. Thus, we can't rely entirely on compile-time checks.

There is a way to do this check only once at program startup though: GNU IFUNC resolver. @zhulipeng did that in this PR:

Optimize string2ll with load-time CPU feature check using IFUNC resolver #2099

zuiderkwast · 2025-09-02T16:19:02Z

If it's only for the BITCOUNT command, I suppose a runtime check is good enough though. It's just a single check in the execution of the command, which is probably negligible.

TedLyngmo · 2025-09-02T16:22:24Z

Well spotted. #1741 was released in 8.1 so we'll need to backport this fix to 8.1.

Please use a more descriptive test in the PRs, rather than only referring to an issue. When we merge, we squash-merge all the commits and use the PR title and description as the commit message. (We don't always need an issue btw. Only a PR is fine.)

Ok, will do!

The same amd64 binary can be used by all amd64 machines, with or without various AVX support. Binary files are often distributed in distros and in containers. Thus, we can't rely entirely on compile-time checks.

I see! It didn't even occur to me that people would use it from distros rather than compiling it from source 😄

zuiderkwast · 2025-09-02T16:26:45Z

How about adding this check in bitcount.c?

 long long serverPopcount(void *s, long count) {
 #if HAVE_X86_SIMD
     /* If length of s >= 256 bits and the CPU supports AVX2,
      * we prefer to use the SIMD version */
-    if (count >= 32) {
+    if (count >= 32 && __builtin_cpu_supports("avx2")) {
         return popcountAVX2(s, count);
     }
 #endif

TedLyngmo · 2025-09-02T16:29:42Z

How about adding this check in bitcount.c?

-    if (count >= 32) {
+    if (count >= 32 && __builtin_cpu_supports("avx2")) {

That seems to do the trick! I'll let all tests finish before updating the PR.

Edit: All tests passed now!

bitops.c: serverPopcount() used popcountAVX2(), which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error. Added a __builtin_cpu_supports("avx2") check and falling back to the platform agnostic version if AVX2 is not supported. Fixes valkey-io#2570 Signed-off-by: Ted Lyngmo <[email protected]>

roshkhatri

LGTM

src/bitops.c

ranshid · 2025-09-03T07:02:26Z

src/bitops.c

    /* If length of s >= 256 bits and the CPU supports AVX2,
     * we prefer to use the SIMD version */
-    if (count >= 32) {
+    if (count >= 32 && __builtin_cpu_supports("avx2")) {


since I do not expect avx2 support to be dynamically disabled on a CPU, you could use static storage for that and only calculate once when this function is called

Suggested change

if (count >= 32 && __builtin_cpu_supports("avx2")) {

static bool cpu_supports_avx2 = __builtin_cpu_supports("avx2");

if (count >= 32 && cpu_supports_avx2) {

Doesn't work. I tested this with smaller file.

error: initializer element is not constant 5 | static bool cpu_supports_avx2 = __builtin_cpu_supports("avx2"); | ^~~~~~~~~~~~~~~~~~~~~~

It seems the builtin results in test BYTE PTR __cpu_model[rip+13], 4 (https://godbolt.org/z/61vxW7de5), so the assembly expansion of anything __cpuid -related doesn't seem to be there. I can't say for sure if this test is cheaper than using a call_once construct like this: https://godbolt.org/z/TfoG35f13, but the call_once version feels more expensive.

If we could use non-constant initializers for globals, like in C++, the test would become a cmp: https://godbolt.org/z/cxe5M76aM

If the bitwise test is more expensive than cmp we could use __attribute__((constructor)) if that's supported by all implementations valkey aims to support: https://godbolt.org/z/j7oY9qqjY

... but realistically, I can't think of a way to get it any cheaper than what's currently in this PR without trickery.

Interesting. Yes, I believe the CPU id structures are initialized once (by __builtin_cpu_init()) to make the runtime checks faster.

One last idea: We could split the function in multiple implementations to get rid of the runtime check and initialize a function pointer instead: https://godbolt.org/z/713srTrcb
I'm unsure if dereferencing that is cheaper than the test or cmp though.

That's basically the IFUNC resolver approach. We did it for string2ll() in #2099 because it's called many times for every command, e.g. when parsing the RESP command arguments.

Since we need to backport this fix, it's good to keep it minimal.

We can perhaps do the resolver function in a separate PR, if the performance gain is worth the added complexity.

yes. we can wait with that.

also we can just calculate it once in a constructor function and it would also be helpful for other cases:

static bool cpu_supports_avx2; __attribute__((constructor)) static void init_cpu_flags(void) { cpu_supports_avx2 = __builtin_cpu_supports("avx2"); }

or call it from main...

Oh @TedLyngmo I see you already referred to this proposal.

__builtin_cpu_supports("avx2") expands to the same assembly as checking a single variable, so it's just as cheap. It can be seen in the godbolt links posted above.

`bitops.c`: `serverPopcount()` used `popcountAVX2()`, which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error. Added a `__builtin_cpu_supports("avx2")` check and falling back to the platform agnostic version if AVX2 is not supported. Fixes valkey-io#2570 Signed-off-by: Ted Lyngmo <[email protected]>

`bitops.c`: `serverPopcount()` used `popcountAVX2()`, which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error. Added a `__builtin_cpu_supports("avx2")` check and falling back to the platform agnostic version if AVX2 is not supported. Fixes #2570 Signed-off-by: Ted Lyngmo <[email protected]>

`bitops.c`: `serverPopcount()` used `popcountAVX2()`, which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error. Added a `__builtin_cpu_supports("avx2")` check and falling back to the platform agnostic version if AVX2 is not supported. Fixes valkey-io#2570 Signed-off-by: Ted Lyngmo <[email protected]>

`bitops.c`: `serverPopcount()` used `popcountAVX2()`, which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error. Added a `__builtin_cpu_supports("avx2")` check and falling back to the platform agnostic version if AVX2 is not supported. Fixes #2570 Signed-off-by: Ted Lyngmo <[email protected]>

`bitops.c`: `serverPopcount()` used `popcountAVX2()`, which as the name implies requires AVX2 support, on AVX-only machines, causing an "illegal instruction" error. Added a `__builtin_cpu_supports("avx2")` check and falling back to the platform agnostic version if AVX2 is not supported. Fixes valkey-io#2570 Signed-off-by: Ted Lyngmo <[email protected]> Signed-off-by: Harkrishn Patro <[email protected]>

github-actions bot assigned TedLyngmo Sep 2, 2025

ranshid self-requested a review September 2, 2025 15:42

zuiderkwast added this to Valkey 8.1 Sep 2, 2025

zuiderkwast requested a review from zhulipeng September 2, 2025 16:03

zuiderkwast added this to Valkey 9.0 Sep 2, 2025

TedLyngmo force-pushed the avx2 branch from e237cff to 2b9e8e3 Compare September 2, 2025 16:53

zuiderkwast approved these changes Sep 2, 2025

View reviewed changes

roshkhatri approved these changes Sep 2, 2025

View reviewed changes

src/bitops.c Show resolved Hide resolved

ranshid reviewed Sep 3, 2025

View reviewed changes

ranshid approved these changes Sep 3, 2025

View reviewed changes

zuiderkwast merged commit 971cdb7 into valkey-io:unstable Sep 3, 2025
52 checks passed

github-project-automation bot moved this to To be backported in Valkey 8.1 Sep 3, 2025

github-project-automation bot moved this to Done in Valkey 9.0 Sep 3, 2025

TedLyngmo deleted the avx2 branch September 3, 2025 17:03

zuiderkwast added the release-notes This issue should get a line item in the release notes label Sep 4, 2025

ranshid moved this from To be backported to In Progress in Valkey 8.1 Sep 30, 2025

ranshid moved this from In Progress to 8.1.4 in Valkey 8.1 Sep 30, 2025

ranshid moved this from 8.1.4 to To be backported in Valkey 8.1 Sep 30, 2025

zuiderkwast moved this from To be backported to 8.1.4 in Valkey 8.1 Oct 1, 2025

	if (count >= 32 && __builtin_cpu_supports("avx2")) {
	static bool cpu_supports_avx2 = __builtin_cpu_supports("avx2");
	if (count >= 32 && cpu_supports_avx2) {

Don't use AVX2 instructions if the CPU don't support it #2571

Don't use AVX2 instructions if the CPU don't support it #2571

Uh oh!

Conversation

TedLyngmo commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranshid commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TedLyngmo commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Sep 2, 2025

Uh oh!

TedLyngmo commented Sep 2, 2025

Uh oh!

zuiderkwast commented Sep 2, 2025

Uh oh!

TedLyngmo commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roshkhatri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranshid Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

TedLyngmo Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

TedLyngmo Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

ranshid Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranshid Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TedLyngmo commented Sep 2, 2025 •

edited

Loading

codecov bot commented Sep 2, 2025 •

edited

Loading

ranshid commented Sep 2, 2025 •

edited

Loading

TedLyngmo commented Sep 2, 2025 •

edited

Loading

zuiderkwast commented Sep 2, 2025 •

edited

Loading

TedLyngmo commented Sep 2, 2025 •

edited

Loading

ranshid Sep 3, 2025 •

edited

Loading

TedLyngmo Sep 3, 2025 •

edited

Loading

ranshid Sep 3, 2025 •

edited

Loading